Patchwork [1/2] fix virtio_blk serial pci config breakage

login
register
mail settings
Submitter john cooper
Date Sept. 29, 2009, 6:09 a.m.
Message ID <4AC1A4AD.10509@redhat.com>
Download mbox | patch
Permalink /patch/34410/
State Superseded
Headers show

Comments

john cooper - Sept. 29, 2009, 6:09 a.m.
[This patch in part replicates the first version of
this feature which was rolled-back due to breakage
it caused extending the pci config area beyond the
256 byte limit.]

Add missing logic in qemu to accept a command line
serial number parameter for virtio_blk, integrate
it into an ATA IDENTIFY structure, map the resulting
data via PCI BAR #5.

Signed-off-by: john cooper <john.cooper@redhat.com>
---
Michael S. Tsirkin - Sept. 29, 2009, 9:01 a.m.
On Tue, Sep 29, 2009 at 02:09:49AM -0400, john cooper wrote:
> [This patch in part replicates the first version of
> this feature which was rolled-back due to breakage
> it caused extending the pci config area beyond the
> 256 byte limit.]

The patch does not apply on top of qemu git tree.
How should I apply it?

> Add missing logic in qemu to accept a command line
> serial number parameter for virtio_blk, integrate
> it into an ATA IDENTIFY structure, map the resulting
> data via PCI BAR #5.
> 
> Signed-off-by: john cooper <john.cooper@redhat.com>
> ---
> 
> 
> diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
> index dad4ef0..9a067c8 100644
> --- a/hw/virtio-blk.c
> +++ b/hw/virtio-blk.c
> @@ -19,12 +19,18 @@
>  # include <scsi/sg.h>
>  #endif
>  
> +#define VBLK_IDENTIFY_SIZE      512
> +#define VBLK_IDENTIFY_AMASK	(VBLK_IDENTIFY_SIZE - 1)
> +#define VBLK_IDENTIFY_CFGSLOT   5 /* PCI BAR #5 maps identify/config area */
> +
>  typedef struct VirtIOBlock
>  {
>      VirtIODevice vdev;
>      BlockDriverState *bs;
>      VirtQueue *vq;
>      void *rq;
> +    uint32_t mmio_io_addr;
> +    uint16_t identify[VIRTIO_BLK_ID_LEN];
>  } VirtIOBlock;
>  
>  static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
> @@ -32,6 +38,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;
> @@ -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_IDENTIFY;
>  
>      return features;
>  }
> @@ -348,6 +399,60 @@ static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +/* fan-in for VBLK_IDENTIFY_CFGSLOT read operations
> + */
> +static uint32_t virtio_blk_io_read(void *opaque, target_phys_addr_t addr,
> +                                   uint8_t access_size)
> +{
> +    VirtIOBlock *s = opaque;
> +    uint16_t byte_index = addr & VBLK_IDENTIFY_AMASK;
> +    uint8_t *source = &((uint8_t *)s->identify)[byte_index];
> +
> +    switch (access_size)
> +        {
> +    case 1:
> +    	return (*(uint8_t *)source);
> +    case 2:
> +    	return (*(uint16_t *)source);
> +    case 4:
> +    	return (*(uint32_t *)source);
> +        }
> +    return 0;
> +}
> +
> +static uint32_t virtio_blk_mmio_readb(void *opaque, target_phys_addr_t addr)
> +{
> +    return virtio_blk_io_read(opaque, addr, 1);
> +}
> +
> +static uint32_t virtio_blk_mmio_readw(void *opaque, target_phys_addr_t addr)
> +{
> +    return virtio_blk_io_read(opaque, addr, 2);
> +}
> +
> +static uint32_t virtio_blk_mmio_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    return virtio_blk_io_read(opaque, addr, 4);
> +}
> +
> +static CPUReadMemoryFunc *virtio_blk_mmio_read[3] = {
> +    virtio_blk_mmio_readb,
> +    virtio_blk_mmio_readw,
> +    virtio_blk_mmio_readl,
> +};
> +
> +static CPUWriteMemoryFunc *virtio_blk_mmio_write[3] = {
> +    NULL, NULL, NULL,
> +};
> +
> +static void virtio_blk_map(PCIDevice *pci_dev, int region_num, uint32_t addr,
> +                           uint32_t size, int type)
> +{
> +    VirtIOBlock *s = (VirtIOBlock *)pci_dev;
> +
> +    cpu_register_physical_memory(addr, VBLK_IDENTIFY_SIZE, s->mmio_io_addr);
> +}
> +
>  void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
>  {
>      VirtIOBlock *s;
> @@ -360,10 +465,18 @@ 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;
>  
> +    s->mmio_io_addr =
> +        cpu_register_io_memory(0, virtio_blk_mmio_read,
> +                               virtio_blk_mmio_write, s);
> +    pci_register_io_region(&((VirtIODevice *)s)->pci_dev,
> +                           VBLK_IDENTIFY_CFGSLOT, VBLK_IDENTIFY_SIZE,
> +                           PCI_ADDRESS_SPACE_MEM, virtio_blk_map);
> +
>      s->vdev.get_config = virtio_blk_update_config;
>      s->vdev.get_features = virtio_blk_get_features;
>      s->vdev.reset = virtio_blk_reset;
> @@ -373,6 +486,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..1cd5d45 100644
> --- a/hw/virtio-blk.h
> +++ b/hw/virtio-blk.h
> @@ -31,6 +31,11 @@
>  #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       /* ATA IDENTIFY supported */
> +

This bit is already used for the non-PCI-compliant feature.
We need a new one otherwise guests will be confused.

> +#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
>  {
> 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@redhat.com

Patch

diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c
index dad4ef0..9a067c8 100644
--- a/hw/virtio-blk.c
+++ b/hw/virtio-blk.c
@@ -19,12 +19,18 @@ 
 # include <scsi/sg.h>
 #endif
 
+#define VBLK_IDENTIFY_SIZE      512
+#define VBLK_IDENTIFY_AMASK	(VBLK_IDENTIFY_SIZE - 1)
+#define VBLK_IDENTIFY_CFGSLOT   5 /* PCI BAR #5 maps identify/config area */
+
 typedef struct VirtIOBlock
 {
     VirtIODevice vdev;
     BlockDriverState *bs;
     VirtQueue *vq;
     void *rq;
+    uint32_t mmio_io_addr;
+    uint16_t identify[VIRTIO_BLK_ID_LEN];
 } VirtIOBlock;
 
 static VirtIOBlock *to_virtio_blk(VirtIODevice *vdev)
@@ -32,6 +38,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;
@@ -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_IDENTIFY;
 
     return features;
 }
@@ -348,6 +399,60 @@  static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id)
     return 0;
 }
 
+/* fan-in for VBLK_IDENTIFY_CFGSLOT read operations
+ */
+static uint32_t virtio_blk_io_read(void *opaque, target_phys_addr_t addr,
+                                   uint8_t access_size)
+{
+    VirtIOBlock *s = opaque;
+    uint16_t byte_index = addr & VBLK_IDENTIFY_AMASK;
+    uint8_t *source = &((uint8_t *)s->identify)[byte_index];
+
+    switch (access_size)
+        {
+    case 1:
+    	return (*(uint8_t *)source);
+    case 2:
+    	return (*(uint16_t *)source);
+    case 4:
+    	return (*(uint32_t *)source);
+        }
+    return 0;
+}
+
+static uint32_t virtio_blk_mmio_readb(void *opaque, target_phys_addr_t addr)
+{
+    return virtio_blk_io_read(opaque, addr, 1);
+}
+
+static uint32_t virtio_blk_mmio_readw(void *opaque, target_phys_addr_t addr)
+{
+    return virtio_blk_io_read(opaque, addr, 2);
+}
+
+static uint32_t virtio_blk_mmio_readl(void *opaque, target_phys_addr_t addr)
+{
+    return virtio_blk_io_read(opaque, addr, 4);
+}
+
+static CPUReadMemoryFunc *virtio_blk_mmio_read[3] = {
+    virtio_blk_mmio_readb,
+    virtio_blk_mmio_readw,
+    virtio_blk_mmio_readl,
+};
+
+static CPUWriteMemoryFunc *virtio_blk_mmio_write[3] = {
+    NULL, NULL, NULL,
+};
+
+static void virtio_blk_map(PCIDevice *pci_dev, int region_num, uint32_t addr,
+                           uint32_t size, int type)
+{
+    VirtIOBlock *s = (VirtIOBlock *)pci_dev;
+
+    cpu_register_physical_memory(addr, VBLK_IDENTIFY_SIZE, s->mmio_io_addr);
+}
+
 void *virtio_blk_init(PCIBus *bus, BlockDriverState *bs)
 {
     VirtIOBlock *s;
@@ -360,10 +465,18 @@  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;
 
+    s->mmio_io_addr =
+        cpu_register_io_memory(0, virtio_blk_mmio_read,
+                               virtio_blk_mmio_write, s);
+    pci_register_io_region(&((VirtIODevice *)s)->pci_dev,
+                           VBLK_IDENTIFY_CFGSLOT, VBLK_IDENTIFY_SIZE,
+                           PCI_ADDRESS_SPACE_MEM, virtio_blk_map);
+
     s->vdev.get_config = virtio_blk_update_config;
     s->vdev.get_features = virtio_blk_get_features;
     s->vdev.reset = virtio_blk_reset;
@@ -373,6 +486,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..1cd5d45 100644
--- a/hw/virtio-blk.h
+++ b/hw/virtio-blk.h
@@ -31,6 +31,11 @@ 
 #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       /* 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
 {
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