diff mbox

fix virtio_blk serial pci config breakage, v2

Message ID 4ACA1527.9050305@third-harmonic.com
State Superseded
Headers show

Commit Message

john cooper Oct. 5, 2009, 3:47 p.m. UTC 
This is a re-work of the previous version where the
associated data was being funneled through a free
PCI BAR mapping.  Here a request for the identify
information results in a virtqueue command utilizing
the scaffolding introduced by Rusty's recent patch.

Signed-off-by: john cooper <john.cooper@redhat.com>
---

Comments

Michael S. Tsirkin Oct. 5, 2009, 7:54 p.m. UTC | #1 
On Mon, Oct 05, 2009 at 11:47:51AM -0400, john cooper wrote:
> This is a re-work of the previous version where the
> associated data was being funneled through a free
> PCI BAR mapping.  Here a request for the identify
> information results in a virtqueue command utilizing
> the scaffolding introduced by Rusty's recent patch.
>
> Signed-off-by: john cooper <john.cooper@redhat.com>

good stuff. A couple of comments below.
Also, what's going on with text alignment here?

> ---
>
>
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index dad4ef0..e754277 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -25,6 +25,7 @@ typedef struct VirtIOBlock
>     BlockDriverState *bs;
>     VirtQueue *vq;
>     void *rq;
> +    uint16_t identify[VIRTIO_BLK_ID_LEN];
> } VirtIOBlock;
>
> static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> @@ -32,6 +33,48 @@ static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>     return (VirtIOBlock *)vdev;
> }
>
> +/* store identify data in little endian format
> + */
> +static inline void put_le16(uint16_t *p, unsigned int v)
> +{
> +    *p = cpu_to_le16(v);
> +}
> +
> +/* copy to *dst from *src, nul pad dst tail as needed to len bytes
> + */
> +static inline void padstr(char *dst, const char *src, int len)
> +{
> +    while (len--)
> +        *dst++ = *src ? *src++ : '\0';
> +}
> +
> +/* setup simulated identify data as appropriate for virtio block device
> + *
> + * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
> + */
> +static inline void virtio_identify_template(VirtIOBlock *s)
> +{
> +    uint16_t *p = s->identify;
> +    uint64_t lba_sectors;
> +
> +    memset(p, 0, sizeof(uint16_t) * VIRTIO_BLK_ID_LEN);

better as sizeof s->identity

> +    put_le16(p + 0, 0x0);                            /* ATA device */
> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */

QEMU version is currently a string like "0.11.50" which is exactly 8
bytes. What if someone makes it longer?  padstr will not 0
terminate string, and only partial data will be there.
Maybe put compile assert here?

Also, identify is pre-initialized to 0, isn't it?
So just strcpy should be enough, here and elsewhere,
no need to roll our own padstr.

> +    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
> +    put_le16(p + 47, 0x80ff);                        /* max xfer 255 sectors */
> +    put_le16(p + 49, 0x0b00);                        /* support IORDY/LBA/DMA */
> +    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 sectors */
> +    put_le16(p + 80, 0x1f0);                         /* support ATA8/7/6/5/4 */
> +    put_le16(p + 81, 0x16);
> +    put_le16(p + 82, 0x400);
> +    put_le16(p + 83, 0x400);
> +    bdrv_get_geometry(s->bs, &lba_sectors);
> +    put_le16(p + 100, lba_sectors);
> +    put_le16(p + 101, lba_sectors >> 16);
> +    put_le16(p + 102, lba_sectors >> 32);
> +    put_le16(p + 103, lba_sectors >> 48);
> +}
> +
> typedef struct VirtIOBlockReq
> {
>     VirtIOBlock *dev;
> @@ -243,6 +286,11 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>
>         if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
>             virtio_blk_handle_scsi(req);
> +        }
> +        else if (req->out->type & VIRTIO_BLK_T_GET_ID) {

Pls put } and else on the same line

> +            memcpy(req->elem.in_sg[0].iov_base, s->identify,
> +                req->elem.in_sg[0].iov_len);

Is this safe? Can guest make iov_len bigger than size of s->identity?

> +        virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
>         } else if (req->out->type & VIRTIO_BLK_T_OUT) {
>             qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
>                                      req->elem.out_num - 1);
> @@ -304,6 +352,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>
> static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
> {
> +    VirtIOBlock *s = to_virtio_blk(vdev);
>     uint32_t features = 0;
>
>     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
> @@ -311,6 +360,8 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
> #ifdef __linux__
>     features |= (1 << VIRTIO_BLK_F_SCSI);
> #endif
> +    if (*(char *)&s->identify[VIRTIO_BLK_ID_SN])
> +        features |= 1 << VIRTIO_BLK_F_GET_ID;
>     return features;
> }
> @@ -360,7 +411,8 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>                                        PCI_VENDOR_ID_REDHAT_QUMRANET,
>                                        VIRTIO_ID_BLOCK,
>                                        PCI_CLASS_STORAGE_OTHER, 0x00,
> -                                       sizeof(struct virtio_blk_config), sizeof(VirtIOBlock));
> +                                       sizeof(struct virtio_blk_config),
> +                                       sizeof(VirtIOBlock));
>     if (!s)
>         return NULL;
>
> @@ -373,6 +425,10 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
>     bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
>
> +    virtio_identify_template(s);
> +    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
> +        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);

This can silently truncate the serial, can't it?
Maybe check and error out?

> +
>     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
>
>     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
> diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
> index 5ef6c36..f508f20 100644
> --- a/hw/virtio-blk.h
> +++ b/hw/virtio-blk.h
> @@ -31,6 +31,12 @@
> #define VIRTIO_BLK_F_RO         5       /* Disk is read-only */
> #define VIRTIO_BLK_F_BLK_SIZE   6       /* Block size of disk is available*/
> #define VIRTIO_BLK_F_SCSI       7       /* Supports scsi command passthru */
> +#define _VIRTIO_BLK_F_IDENTIFY   8       /* obsolete */

Let's just put it in comment? It should not be used anywhere.

> +#define VIRTIO_BLK_F_GET_ID     10      /* ATA IDENTIFY supported */
> +
> +#define VIRTIO_BLK_ID_LEN       256     /* length of identify u16 array */
> +#define VIRTIO_BLK_ID_SN        10      /* start of char * serial# */
> +#define VIRTIO_BLK_ID_SN_BYTES  20      /* length in bytes of serial# */
>
> struct virtio_blk_config
> {
> @@ -48,6 +54,8 @@ struct virtio_blk_config
>
> /* This bit says it's a scsi command, not an actual read or write. */
> #define VIRTIO_BLK_T_SCSI_CMD   2
> +#define _VIRTIO_BLK_T_FLUSH	4
> +#define VIRTIO_BLK_T_GET_ID	8
>
> /* Barrier before this op. */
> #define VIRTIO_BLK_T_BARRIER    0x80000000
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 78c7637..dc38f59 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -44,6 +44,8 @@
>  * a read-and-acknowledge. */
> #define VIRTIO_PCI_ISR                  19
>
> +/* The remaining space is defined by each driver as the per-driver
> + * configuration space */
> #define VIRTIO_PCI_CONFIG               20
>
> /* Virtio ABI version, if we increment this, we break the guest driver. */
> diff --git a/sysemu.h b/sysemu.h
> index 1f45fd6..185b4e3 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -141,6 +141,8 @@ typedef enum {
>     BLOCK_ERR_STOP_ANY
> } BlockInterfaceErrorAction;
>
> +#define BLOCK_SERIAL_STRLEN 20
> +
> typedef struct DriveInfo {
>     BlockDriverState *bdrv;
>     BlockInterfaceType type;
> @@ -149,7 +151,7 @@ typedef struct DriveInfo {
>     int used;
>     int drive_opt_idx;
>     BlockInterfaceErrorAction onerror;
> -    char serial[21];
> +    char serial[BLOCK_SERIAL_STRLEN + 1];
> } DriveInfo;
>
> #define MAX_IDE_DEVS	2
>
>
> -- 
> john.cooper@third-harmonic.com
Michael S. Tsirkin Oct. 5, 2009, 8:15 p.m. UTC | #2 
On Mon, Oct 05, 2009 at 11:47:51AM -0400, john cooper wrote:
> This is a re-work of the previous version where the
> associated data was being funneled through a free
> PCI BAR mapping.  Here a request for the identify
> information results in a virtqueue command utilizing
> the scaffolding introduced by Rusty's recent patch.
>
> Signed-off-by: john cooper <john.cooper@redhat.com>

On top of this, there should be a patch removing identity from io bar.
Right? Otherwise we'd still be non-spec-compliant when identity is set.

> ---
>
>
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index dad4ef0..e754277 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -25,6 +25,7 @@ typedef struct VirtIOBlock
>     BlockDriverState *bs;
>     VirtQueue *vq;
>     void *rq;
> +    uint16_t identify[VIRTIO_BLK_ID_LEN];
> } VirtIOBlock;
>
> static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> @@ -32,6 +33,48 @@ static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>     return (VirtIOBlock *)vdev;
> }
>
> +/* store identify data in little endian format
> + */
> +static inline void put_le16(uint16_t *p, unsigned int v)
> +{
> +    *p = cpu_to_le16(v);
> +}
> +
> +/* copy to *dst from *src, nul pad dst tail as needed to len bytes
> + */
> +static inline void padstr(char *dst, const char *src, int len)
> +{
> +    while (len--)
> +        *dst++ = *src ? *src++ : '\0';
> +}
> +
> +/* setup simulated identify data as appropriate for virtio block device
> + *
> + * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
> + */
> +static inline void virtio_identify_template(VirtIOBlock *s)
> +{
> +    uint16_t *p = s->identify;
> +    uint64_t lba_sectors;
> +
> +    memset(p, 0, sizeof(uint16_t) * VIRTIO_BLK_ID_LEN);
> +    put_le16(p + 0, 0x0);                            /* ATA device */
> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
> +    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
> +    put_le16(p + 47, 0x80ff);                        /* max xfer 255 sectors */
> +    put_le16(p + 49, 0x0b00);                        /* support IORDY/LBA/DMA */
> +    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 sectors */
> +    put_le16(p + 80, 0x1f0);                         /* support ATA8/7/6/5/4 */
> +    put_le16(p + 81, 0x16);
> +    put_le16(p + 82, 0x400);
> +    put_le16(p + 83, 0x400);
> +    bdrv_get_geometry(s->bs, &lba_sectors);
> +    put_le16(p + 100, lba_sectors);
> +    put_le16(p + 101, lba_sectors >> 16);
> +    put_le16(p + 102, lba_sectors >> 32);
> +    put_le16(p + 103, lba_sectors >> 48);
> +}
> +
> typedef struct VirtIOBlockReq
> {
>     VirtIOBlock *dev;
> @@ -243,6 +286,11 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>
>         if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
>             virtio_blk_handle_scsi(req);
> +        }
> +        else if (req->out->type & VIRTIO_BLK_T_GET_ID) {
> +            memcpy(req->elem.in_sg[0].iov_base, s->identify,
> +                req->elem.in_sg[0].iov_len);
> +        virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
>         } else if (req->out->type & VIRTIO_BLK_T_OUT) {
>             qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
>                                      req->elem.out_num - 1);
> @@ -304,6 +352,7 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>
> static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
> {
> +    VirtIOBlock *s = to_virtio_blk(vdev);
>     uint32_t features = 0;
>
>     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
> @@ -311,6 +360,8 @@ static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
> #ifdef __linux__
>     features |= (1 << VIRTIO_BLK_F_SCSI);
> #endif
> +    if (*(char *)&s->identify[VIRTIO_BLK_ID_SN])
> +        features |= 1 << VIRTIO_BLK_F_GET_ID;
>
>     return features;
> }
> @@ -360,7 +411,8 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>                                        PCI_VENDOR_ID_REDHAT_QUMRANET,
>                                        VIRTIO_ID_BLOCK,
>                                        PCI_CLASS_STORAGE_OTHER, 0x00,
> -                                       sizeof(struct virtio_blk_config), sizeof(VirtIOBlock));
> +                                       sizeof(struct virtio_blk_config),
> +                                       sizeof(VirtIOBlock));
>     if (!s)
>         return NULL;
>
> @@ -373,6 +425,10 @@ void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
>     bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
>
> +    virtio_identify_template(s);
> +    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
> +        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);
> +
>     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
>
>     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
> diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
> index 5ef6c36..f508f20 100644
> --- a/hw/virtio-blk.h
> +++ b/hw/virtio-blk.h
> @@ -31,6 +31,12 @@
> #define VIRTIO_BLK_F_RO         5       /* Disk is read-only */
> #define VIRTIO_BLK_F_BLK_SIZE   6       /* Block size of disk is available*/
> #define VIRTIO_BLK_F_SCSI       7       /* Supports scsi command passthru */
> +#define _VIRTIO_BLK_F_IDENTIFY   8       /* obsolete */
> +#define VIRTIO_BLK_F_GET_ID     10      /* ATA IDENTIFY supported */
> +
> +#define VIRTIO_BLK_ID_LEN       256     /* length of identify u16 array */
> +#define VIRTIO_BLK_ID_SN        10      /* start of char * serial# */
> +#define VIRTIO_BLK_ID_SN_BYTES  20      /* length in bytes of serial# */
>
> struct virtio_blk_config
> {
> @@ -48,6 +54,8 @@ struct virtio_blk_config
>
> /* This bit says it's a scsi command, not an actual read or write. */
> #define VIRTIO_BLK_T_SCSI_CMD   2
> +#define _VIRTIO_BLK_T_FLUSH	4
> +#define VIRTIO_BLK_T_GET_ID	8
>
> /* Barrier before this op. */
> #define VIRTIO_BLK_T_BARRIER    0x80000000
> diff --git a/hw/virtio.c b/hw/virtio.c
> index 78c7637..dc38f59 100644
> --- a/hw/virtio.c
> +++ b/hw/virtio.c
> @@ -44,6 +44,8 @@
>  * a read-and-acknowledge. */
> #define VIRTIO_PCI_ISR                  19
>
> +/* The remaining space is defined by each driver as the per-driver
> + * configuration space */
> #define VIRTIO_PCI_CONFIG               20
>
> /* Virtio ABI version, if we increment this, we break the guest driver. */
> diff --git a/sysemu.h b/sysemu.h
> index 1f45fd6..185b4e3 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -141,6 +141,8 @@ typedef enum {
>     BLOCK_ERR_STOP_ANY
> } BlockInterfaceErrorAction;
>
> +#define BLOCK_SERIAL_STRLEN 20
> +
> typedef struct DriveInfo {
>     BlockDriverState *bdrv;
>     BlockInterfaceType type;
> @@ -149,7 +151,7 @@ typedef struct DriveInfo {
>     int used;
>     int drive_opt_idx;
>     BlockInterfaceErrorAction onerror;
> -    char serial[21];
> +    char serial[BLOCK_SERIAL_STRLEN + 1];
> } DriveInfo;
>
> #define MAX_IDE_DEVS	2
>
>
> -- 
> john.cooper@third-harmonic.com
Anthony Liguori Oct. 6, 2009, 2:23 p.m. UTC | #3 
john cooper wrote:
> This is a re-work of the previous version where the
> associated data was being funneled through a free
> PCI BAR mapping.  Here a request for the identify
> information results in a virtqueue command utilizing
> the scaffolding introduced by Rusty's recent patch.
>
> Signed-off-by: john cooper <john.cooper@redhat.com>
> ---
>
>
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index dad4ef0..e754277 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -25,6 +25,7 @@ typedef struct VirtIOBlock
>     BlockDriverState *bs;
>     VirtQueue *vq;
>     void *rq;
> +    uint16_t identify[VIRTIO_BLK_ID_LEN];
> } VirtIOBlock;
>
> static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> @@ -32,6 +33,48 @@ static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
>     return (VirtIOBlock *)vdev;
> }
>
> +/* store identify data in little endian format
> + */
> +static inline void put_le16(uint16_t *p, unsigned int v)
> +{
> +    *p = cpu_to_le16(v);
> +}
> +
> +/* copy to *dst from *src, nul pad dst tail as needed to len bytes
> + */
> +static inline void padstr(char *dst, const char *src, int len)
> +{
> +    while (len--)
> +        *dst++ = *src ? *src++ : '\0';
> +}
> +
> +/* setup simulated identify data as appropriate for virtio block device
> + *
> + * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
> + */
> +static inline void virtio_identify_template(VirtIOBlock *s)
> +{
> +    uint16_t *p = s->identify;
> +    uint64_t lba_sectors;
> +
> +    memset(p, 0, sizeof(uint16_t) * VIRTIO_BLK_ID_LEN);
> +    put_le16(p + 0, 0x0);                            /* ATA device */
> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware 
> revision */
> +    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
> +    put_le16(p + 47, 0x80ff);                        /* max xfer 255 
> sectors */
> +    put_le16(p + 49, 0x0b00);                        /* support 
> IORDY/LBA/DMA */
> +    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 
> sectors */
> +    put_le16(p + 80, 0x1f0);                         /* support 
> ATA8/7/6/5/4 */
> +    put_le16(p + 81, 0x16);
> +    put_le16(p + 82, 0x400);
> +    put_le16(p + 83, 0x400);
> +    bdrv_get_geometry(s->bs, &lba_sectors);
> +    put_le16(p + 100, lba_sectors);
> +    put_le16(p + 101, lba_sectors >> 16);
> +    put_le16(p + 102, lba_sectors >> 32);
> +    put_le16(p + 103, lba_sectors >> 48);
> +}
> +
> typedef struct VirtIOBlockReq
> {
>     VirtIOBlock *dev;
> @@ -243,6 +286,11 @@ static void virtio_blk_handle_output(VirtIODevice 
> *vdev, VirtQueue *vq)
>
>         if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
>             virtio_blk_handle_scsi(req);
> +        }
> +        else if (req->out->type & VIRTIO_BLK_T_GET_ID) {

CodingStyle.

> +            memcpy(req->elem.in_sg[0].iov_base, s->identify,
> +                req->elem.in_sg[0].iov_len);
> +        virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);

Weird indentation.

Regards,

Anthony Liguori
john cooper Oct. 7, 2009, 5:49 a.m. UTC | #4 
Michael S. Tsirkin wrote:
>> +    put_le16(p + 0, 0x0);                            /* ATA device */
>> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
> 
> QEMU version is currently a string like "0.11.50" which is exactly 8
> bytes. What if someone makes it longer?  padstr will not 0
> terminate string, and only partial data will be there.

This code treats the field similar to the logic from which
it derives (hw/ide.c) in that the field need not be nul
terminated.  Quiet truncation to 8 bytes can occur here
and in the existing usage but in a practical sense I don't
see much of a recourse.  We can flag a warning but the
data is realistically a best-effort attempt to provide
relevant information in this field.  IOW overflowing
this field probably isn't justification alone to modify
a too long qemu version string.

> Also, identify is pre-initialized to 0, isn't it?
> So just strcpy should be enough, here and elsewhere,
> no need to roll our own padstr.

Actually this is an oversight in the local padstr() which
should be padding the balance of the field with ' ' vs. '\0'.

>> +            memcpy(req->elem.in_sg[0].iov_base, s->identify,
>> +                req->elem.in_sg[0].iov_len);
> 
> Is this safe? Can guest make iov_len bigger than size of s->identity?

Good point, a malicious/buggy guest can.  The memcpy
length should be capped. 

>> +    virtio_identify_template(s);
>> +    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
>> +        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);
> 
> This can silently truncate the serial, can't it?

Yes, it is the same disposition as ide/scsi's treatment
of the S/N.  My concern was of keeping the behavior
consistent.

Thanks,

-john
Anthony Liguori Oct. 7, 2009, 1:48 p.m. UTC | #5 
john cooper wrote:
> Michael S. Tsirkin wrote:
>   
>>> +    put_le16(p + 0, 0x0);                            /* ATA device */
>>> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
>>>       
>> QEMU version is currently a string like "0.11.50" which is exactly 8
>> bytes. What if someone makes it longer?  padstr will not 0
>> terminate string, and only partial data will be there.
>>     
>
> This code treats the field similar to the logic from which
> it derives (hw/ide.c) in that the field need not be nul
> terminated.  Quiet truncation to 8 bytes can occur here
> and in the existing usage but in a practical sense I don't
> see much of a recourse.  We can flag a warning but the
> data is realistically a best-effort attempt to provide
> relevant information in this field.  IOW overflowing
> this field probably isn't justification alone to modify
> a too long qemu version string.
>   

Hrm, we really shouldn't be exposing a version string to the guest in 
the first place.

That's a compatibility issue.

Really, I strongly dislike passing this identity page via virtio.  Why 
are we still going this route instead of just passing the S/N?

Regards,

Anthony Liguori
Michael S. Tsirkin Oct. 7, 2009, 1:52 p.m. UTC | #6 
On Wed, Oct 07, 2009 at 08:48:32AM -0500, Anthony Liguori wrote:
> john cooper wrote:
>> Michael S. Tsirkin wrote:
>>   
>>>> +    put_le16(p + 0, 0x0);                            /* ATA device */
>>>> +    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
>>>>       
>>> QEMU version is currently a string like "0.11.50" which is exactly 8
>>> bytes. What if someone makes it longer?  padstr will not 0
>>> terminate string, and only partial data will be there.
>>>     
>>
>> This code treats the field similar to the logic from which
>> it derives (hw/ide.c) in that the field need not be nul
>> terminated.  Quiet truncation to 8 bytes can occur here
>> and in the existing usage but in a practical sense I don't
>> see much of a recourse.  We can flag a warning but the
>> data is realistically a best-effort attempt to provide
>> relevant information in this field.  IOW overflowing
>> this field probably isn't justification alone to modify
>> a too long qemu version string.
>>   
>
> Hrm, we really shouldn't be exposing a version string to the guest in  
> the first place.
>
> That's a compatibility issue.

Actually, it's a good point. Otherwise e.g. the identity changes with
migration. My understanding is that this isn't the only place where
we do this?

> Really, I strongly dislike passing this identity page via virtio.  Why  
> are we still going this route instead of just passing the S/N?

No opinion on this.

> Regards,
>
> Anthony Liguori
Anthony Liguori Oct. 7, 2009, 1:55 p.m. UTC | #7 
Michael S. Tsirkin wrote:
> Actually, it's a good point. Otherwise e.g. the identity changes with
> migration. My understanding is that this isn't the only place where
> we do this?
>   

Right, we'll need to fix this in the IDE emulation.  I assume we do 
something like that in SCSI also.

Regards,

Anthony Liguori
john cooper Oct. 7, 2009, 3:38 p.m. UTC | #8 
Anthony Liguori wrote:
> Really, I strongly dislike passing this identity page via virtio.  Why
> are we still going this route instead of just passing the S/N?

I believe we've accumulated enough justification to
abandon use of the ata identify interface.

Anthony Liguori wrote:
> Right, we'll need to fix this in the IDE emulation.  I assume we do
> something like that in SCSI also.

Yes, unfortunately that code is stuffing 4 chars
of QEMU_VERSION into the return of an inquiry
command.  It appears this interface as well could
use some attention.

-john
diff mbox

Patch

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index dad4ef0..e754277 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -25,6 +25,7 @@  typedef struct VirtIOBlock
     BlockDriverState *bs;
     VirtQueue *vq;
     void *rq;
+    uint16_t identify[VIRTIO_BLK_ID_LEN];
 } VirtIOBlock;
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
@@ -32,6 +33,48 @@  static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
     return (VirtIOBlock *)vdev;
 }
 
+/* store identify data in little endian format
+ */
+static inline void put_le16(uint16_t *p, unsigned int v)
+{
+    *p = cpu_to_le16(v);
+}
+
+/* copy to *dst from *src, nul pad dst tail as needed to len bytes
+ */
+static inline void padstr(char *dst, const char *src, int len)
+{
+    while (len--)
+        *dst++ = *src ? *src++ : '\0';
+}
+
+/* setup simulated identify data as appropriate for virtio block device
+ *
+ * ref: AT Attachment 8 - ATA/ATAPI Command Set (ATA8-ACS)
+ */
+static inline void virtio_identify_template(VirtIOBlock *s)
+{
+    uint16_t *p = s->identify;
+    uint64_t lba_sectors;
+
+    memset(p, 0, sizeof(uint16_t) * VIRTIO_BLK_ID_LEN);
+    put_le16(p + 0, 0x0);                            /* ATA device */
+    padstr((char *)(p + 23), QEMU_VERSION, 8);       /* firmware revision */
+    padstr((char *)(p + 27), "QEMU VIRT_BLK", 40);   /* model# */
+    put_le16(p + 47, 0x80ff);                        /* max xfer 255 sectors */
+    put_le16(p + 49, 0x0b00);                        /* support IORDY/LBA/DMA */
+    put_le16(p + 59, 0x1ff);                         /* cur xfer 255 sectors */
+    put_le16(p + 80, 0x1f0);                         /* support ATA8/7/6/5/4 */
+    put_le16(p + 81, 0x16);
+    put_le16(p + 82, 0x400);
+    put_le16(p + 83, 0x400);
+    bdrv_get_geometry(s->bs, &lba_sectors);
+    put_le16(p + 100, lba_sectors);
+    put_le16(p + 101, lba_sectors >> 16);
+    put_le16(p + 102, lba_sectors >> 32);
+    put_le16(p + 103, lba_sectors >> 48);
+}
+
 typedef struct VirtIOBlockReq
 {
     VirtIOBlock *dev;
@@ -243,6 +286,11 @@  static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 
         if (req->out->type & VIRTIO_BLK_T_SCSI_CMD) {
             virtio_blk_handle_scsi(req);
+        }
+        else if (req->out->type & VIRTIO_BLK_T_GET_ID) {
+            memcpy(req->elem.in_sg[0].iov_base, s->identify,
+                req->elem.in_sg[0].iov_len);
+        virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
         } else if (req->out->type & VIRTIO_BLK_T_OUT) {
             qemu_iovec_init_external(&req->qiov, &req->elem.out_sg[1],
                                      req->elem.out_num - 1);
@@ -304,6 +352,7 @@  static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
 static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 {
+    VirtIOBlock *s = to_virtio_blk(vdev);
     uint32_t features = 0;
 
     features |= (1 << VIRTIO_BLK_F_SEG_MAX);
@@ -311,6 +360,8 @@  static uint32_t virtio_blk_get_features(VirtIODevice *vdev)
 #ifdef __linux__
     features |= (1 << VIRTIO_BLK_F_SCSI);
 #endif
+    if (*(char *)&s->identify[VIRTIO_BLK_ID_SN])
+        features |= 1 << VIRTIO_BLK_F_GET_ID;
 
     return features;
 }
@@ -360,7 +411,8 @@  void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
                                        PCI_VENDOR_ID_REDHAT_QUMRANET,
                                        VIRTIO_ID_BLOCK,
                                        PCI_CLASS_STORAGE_OTHER, 0x00,
-                                       sizeof(struct virtio_blk_config), sizeof(VirtIOBlock));
+                                       sizeof(struct virtio_blk_config),
+                                       sizeof(VirtIOBlock));
     if (!s)
         return NULL;
 
@@ -373,6 +425,10 @@  void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
     bdrv_guess_geometry(s->bs, &cylinders, &heads, &secs);
     bdrv_set_geometry_hint(s->bs, cylinders, heads, secs);
 
+    virtio_identify_template(s);
+    strncpy((char *)&s->identify[VIRTIO_BLK_ID_SN],
+        (char *)drive_get_serial(bs), VIRTIO_BLK_ID_SN_BYTES);
+
     s->vq = virtio_add_queue(&s->vdev, 128, virtio_blk_handle_output);
 
     qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
diff --git a/hw/virtio-blk.h b/hw/virtio-blk.h
index 5ef6c36..f508f20 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -31,6 +31,12 @@ 
 #define VIRTIO_BLK_F_RO         5       /* Disk is read-only */
 #define VIRTIO_BLK_F_BLK_SIZE   6       /* Block size of disk is available*/
 #define VIRTIO_BLK_F_SCSI       7       /* Supports scsi command passthru */
+#define _VIRTIO_BLK_F_IDENTIFY   8       /* obsolete */
+#define VIRTIO_BLK_F_GET_ID     10      /* ATA IDENTIFY supported */
+
+#define VIRTIO_BLK_ID_LEN       256     /* length of identify u16 array */
+#define VIRTIO_BLK_ID_SN        10      /* start of char * serial# */
+#define VIRTIO_BLK_ID_SN_BYTES  20      /* length in bytes of serial# */
 
 struct virtio_blk_config
 {
@@ -48,6 +54,8 @@  struct virtio_blk_config
 
 /* This bit says it's a scsi command, not an actual read or write. */
 #define VIRTIO_BLK_T_SCSI_CMD   2
+#define _VIRTIO_BLK_T_FLUSH	4
+#define VIRTIO_BLK_T_GET_ID	8
 
 /* Barrier before this op. */
 #define VIRTIO_BLK_T_BARRIER    0x80000000
diff --git a/hw/virtio.c b/hw/virtio.c
index 78c7637..dc38f59 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -44,6 +44,8 @@ 
  * a read-and-acknowledge. */
 #define VIRTIO_PCI_ISR                  19
 
+/* The remaining space is defined by each driver as the per-driver
+ * configuration space */
 #define VIRTIO_PCI_CONFIG               20
 
 /* Virtio ABI version, if we increment this, we break the guest driver. */
diff --git a/sysemu.h b/sysemu.h
index 1f45fd6..185b4e3 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -141,6 +141,8 @@  typedef enum {
     BLOCK_ERR_STOP_ANY
 } BlockInterfaceErrorAction;
 
+#define BLOCK_SERIAL_STRLEN 20
+
 typedef struct DriveInfo {
     BlockDriverState *bdrv;
     BlockInterfaceType type;
@@ -149,7 +151,7 @@  typedef struct DriveInfo {
     int used;
     int drive_opt_idx;
     BlockInterfaceErrorAction onerror;
-    char serial[21];
+    char serial[BLOCK_SERIAL_STRLEN + 1];
 } DriveInfo;
 
 #define MAX_IDE_DEVS	2