Message ID | 1407950470-26183-1-git-send-email-jsnow@redhat.com |
---|---|
State | New |
Headers | show |
John Snow <jsnow@redhat.com> writes: > Currently, if the block device backing the IDE drive is resized, > the information about the device as cached inside of the IDEState > structure is not updated, thus when a guest OS re-queries the drive, > it is unable to see the expanded size. > > This patch adds a resize callback that updates the IDENTIFY data > buffer in order to correct this. > > Lastly, a Linux guest as-is cannot resize a libata drive while in-use, > but it can see the expanded size as part of a bus rescan event. > This patch also allows guests such as Linux to see the new drive size > after a soft reboot event, without having to exit the QEMU process. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > > V2: > - Do not attempt to update geometry values, to avoid clobbering > user-specified values, if they exist. > - Do not regenerate the entire IDENTIFY buffer to avoid losing > any settings that occurred during normal operation. > > hw/ide/core.c | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/hw/ide/core.c b/hw/ide/core.c > index db191a6..ba5e3ad 100644 > --- a/hw/ide/core.c > +++ b/hw/ide/core.c > @@ -2099,6 +2099,33 @@ static bool ide_cd_is_medium_locked(void *opaque) > return ((IDEState *)opaque)->tray_locked; > } > > +static void ide_resize_cb(void *opaque) > +{ > + IDEState *s = opaque; > + uint16_t *p = (uint16_t *)s->identify_data; > + uint64_t nb_sectors; > + > + if (!s->identify_set) { > + return; > + } > + > + bdrv_get_geometry(s->bs, &nb_sectors); > + s->nb_sectors = nb_sectors; > + > + /* Update the identify buffer. Note, this is not called for ATAPI. */ > + if (s->drive_kind != IDE_CFATA) { > + put_le16(p + 60, s->nb_sectors); > + put_le16(p + 61, s->nb_sectors >> 16); > + put_le16(p + 100, s->nb_sectors); > + put_le16(p + 101, s->nb_sectors >> 16); > + put_le16(p + 102, s->nb_sectors >> 32); > + put_le16(p + 103, s->nb_sectors >> 48); > + } else { > + put_le16(p + 7, s->nb_sectors >> 16); > + put_le16(p + 8, s->nb_sectors); > + } I'd prefer if (s->drive_kind == IDE_CFATA) ... else ..., because it avoids the double negative. Your code duplicates the parts of the functions initializing s->identify_data dealing with nb_sectors. These are: * ide_identify() for IDE_HD Writes nb_sectors to slots 60..61 and 100..103. Matches your code. * ide_atapi_identify() for IDE_CD Doesn't use it at all. Your code clobbers slots 60..61 and 100.103. Oops. * ide_cfata_identify() for IDE_CFATA Writes nb_sectors to slots 7..8 and and 60..61. Your code only writes the former. I guess code duplication is tolerable, because the format of s->identify_data is unlikely to change. Comments linking the copies together wouldn't hurt, though. If we don't want the duplication, we need to factor the length code out of the identify functions, so we can use it in both places. [...]
On 08/14/2014 03:12 AM, Markus Armbruster wrote: > > I'd prefer if (s->drive_kind == IDE_CFATA) ... else ..., because it > avoids the double negative. OK. This is how cmd_identify delegates. For matters of style I usually try to defer to nearby code. > > Your code duplicates the parts of the functions initializing > s->identify_data dealing with nb_sectors. These are: > > * ide_identify() for IDE_HD > > Writes nb_sectors to slots 60..61 and 100..103. Matches your code. > > * ide_atapi_identify() for IDE_CD > > Doesn't use it at all. Your code clobbers slots 60..61 and 100.103. > Oops. > ide_resize_cb does not get called for ATAPI drives, so I think this is not relevant. I tried to note this in a comment. See ide_init_drive: if (kind == IDE_CD) { bdrv_set_dev_ops(bs, &ide_cd_block_ops, s); ... } else { ... bdrv_set_dev_ops(bs, &ide_hd_block_ops, s); } > * ide_cfata_identify() for IDE_CFATA > > Writes nb_sectors to slots 7..8 and and 60..61. Your code only writes > the former. ...Sorry. I appreciate you taking your time to review these patches. I will submit a V3. > > I guess code duplication is tolerable, because the format of > s->identify_data is unlikely to change. Comments linking the copies > together wouldn't hurt, though. > > If we don't want the duplication, we need to factor the length code out > of the identify functions, so we can use it in both places. > The length and complexity didn't feel like it needed to be factored out into two new functions, and I liked the idea of keeping the writes to the buffer sequential inside the initialization functions, because it made it easier for me to read along with the spec that way. Factoring it out seemed more of a fruitless hassle than anything. Thanks, --John
John Snow <jsnow@redhat.com> writes: > On 08/14/2014 03:12 AM, Markus Armbruster wrote: >> >> I'd prefer if (s->drive_kind == IDE_CFATA) ... else ..., because it >> avoids the double negative. > > OK. This is how cmd_identify delegates. For matters of style I usually > try to defer to nearby code. > >> >> Your code duplicates the parts of the functions initializing >> s->identify_data dealing with nb_sectors. These are: >> >> * ide_identify() for IDE_HD >> >> Writes nb_sectors to slots 60..61 and 100..103. Matches your code. >> >> * ide_atapi_identify() for IDE_CD >> >> Doesn't use it at all. Your code clobbers slots 60..61 and 100.103. >> Oops. >> > > ide_resize_cb does not get called for ATAPI drives, so I think this is > not relevant. I tried to note this in a comment. See ide_init_drive: > > if (kind == IDE_CD) { > bdrv_set_dev_ops(bs, &ide_cd_block_ops, s); > ... > } else { > ... > bdrv_set_dev_ops(bs, &ide_hd_block_ops, s); > } You're right. You could assert it in ide_resize_cb(), like this: if (s->drive_kind == IDE_HD) { .... } else { assert(s->drive_kind == IDE_CFATA); ... } >> * ide_cfata_identify() for IDE_CFATA >> >> Writes nb_sectors to slots 7..8 and and 60..61. Your code only writes >> the former. > > ...Sorry. I appreciate you taking your time to review these patches. I > will submit a V3. No problem, we're all human. >> I guess code duplication is tolerable, because the format of >> s->identify_data is unlikely to change. Comments linking the copies >> together wouldn't hurt, though. >> >> If we don't want the duplication, we need to factor the length code out >> of the identify functions, so we can use it in both places. >> > > The length and complexity didn't feel like it needed to be factored > out into two new functions, and I liked the idea of keeping the writes > to the buffer sequential inside the initialization functions, because > it made it easier for me to read along with the spec that > way. Factoring it out seemed more of a fruitless hassle than anything. You're probably right.
diff --git a/hw/ide/core.c b/hw/ide/core.c index db191a6..ba5e3ad 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -2099,6 +2099,33 @@ static bool ide_cd_is_medium_locked(void *opaque) return ((IDEState *)opaque)->tray_locked; } +static void ide_resize_cb(void *opaque) +{ + IDEState *s = opaque; + uint16_t *p = (uint16_t *)s->identify_data; + uint64_t nb_sectors; + + if (!s->identify_set) { + return; + } + + bdrv_get_geometry(s->bs, &nb_sectors); + s->nb_sectors = nb_sectors; + + /* Update the identify buffer. Note, this is not called for ATAPI. */ + if (s->drive_kind != IDE_CFATA) { + put_le16(p + 60, s->nb_sectors); + put_le16(p + 61, s->nb_sectors >> 16); + put_le16(p + 100, s->nb_sectors); + put_le16(p + 101, s->nb_sectors >> 16); + put_le16(p + 102, s->nb_sectors >> 32); + put_le16(p + 103, s->nb_sectors >> 48); + } else { + put_le16(p + 7, s->nb_sectors >> 16); + put_le16(p + 8, s->nb_sectors); + } +} + static const BlockDevOps ide_cd_block_ops = { .change_media_cb = ide_cd_change_cb, .eject_request_cb = ide_cd_eject_request_cb, @@ -2106,6 +2133,10 @@ static const BlockDevOps ide_cd_block_ops = { .is_medium_locked = ide_cd_is_medium_locked, }; +static const BlockDevOps ide_hd_block_ops = { + .resize_cb = ide_resize_cb, +}; + int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, const char *version, const char *serial, const char *model, uint64_t wwn, @@ -2142,6 +2173,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind, error_report("Can't use a read-only drive"); return -1; } + bdrv_set_dev_ops(bs, &ide_hd_block_ops, s); } if (serial) { pstrcpy(s->drive_serial_str, sizeof(s->drive_serial_str), serial);
Currently, if the block device backing the IDE drive is resized, the information about the device as cached inside of the IDEState structure is not updated, thus when a guest OS re-queries the drive, it is unable to see the expanded size. This patch adds a resize callback that updates the IDENTIFY data buffer in order to correct this. Lastly, a Linux guest as-is cannot resize a libata drive while in-use, but it can see the expanded size as part of a bus rescan event. This patch also allows guests such as Linux to see the new drive size after a soft reboot event, without having to exit the QEMU process. Signed-off-by: John Snow <jsnow@redhat.com> --- V2: - Do not attempt to update geometry values, to avoid clobbering user-specified values, if they exist. - Do not regenerate the entire IDENTIFY buffer to avoid losing any settings that occurred during normal operation. hw/ide/core.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)