[PATCH] add write support to virtio-blk

[Gleb Natapov]

Windows XP needs it during installation.

Signed-off-by: Gleb Natapov 
diff --git a/src/virtio-blk.c b/src/virtio-blk.c
index 74af488..91277f4 100644
--- a/src/virtio-blk.c
+++ b/src/virtio-blk.c
@@ -26,13 +26,13 @@ struct virtiodrive_s {
 };

static int
-virtio_blk_read(struct disk_op_s *op)
+virtio_blk_op(struct disk_op_s *op, int write)
 {
     struct virtiodrive_s *vdrive_g =
         container_of(op->drive_g, struct virtiodrive_s, drive);
     struct vring_virtqueue *vq = GET_GLOBAL(vdrive_g->vq);
     struct virtio_blk_outhdr hdr = {
-        .type = VIRTIO_BLK_T_IN,
+        .type = write ? VIRTIO_BLK_T_OUT : VIRTIO_BLK_T_IN,
         .ioprio = 0,
         .sector = op->lba,
     };
@@ -78,9 +78,10 @@ process_virtio_op(struct disk_op_s *op)
         return 0;
     switch (op->command) {
     case CMD_READ:
-        return virtio_blk_read(op);
-    case CMD_FORMAT:
+        return virtio_blk_op(op, 0);
     case CMD_WRITE:
+        return virtio_blk_op(op, 1);
+    case CMD_FORMAT:
         return DISK_RET_EWRITEPROTECT;
     case CMD_RESET:
     case CMD_ISREADY:
--
    		Gleb.

[PATCHv2] add write support to virtio-blk

[Gleb Natapov]

Windows XP does write to sector 0 during installation and prints
mysterious error if write fails. Interestingly if write drops data,
but returns OK to Windows installer installation proceed without
complains and completes successfully.

Signed-off-by: Gleb Natapov 
---
v1->v2
  Call to vring_add_buf() should be different for write and read.

diff --git a/src/virtio-blk.c b/src/virtio-blk.c
index 74af488..9835267 100644
--- a/src/virtio-blk.c
+++ b/src/virtio-blk.c
@@ -26,13 +26,13 @@ struct virtiodrive_s {
 };

static int
-virtio_blk_read(struct disk_op_s *op)
+virtio_blk_op(struct disk_op_s *op, int write)
 {
     struct virtiodrive_s *vdrive_g =
         container_of(op->drive_g, struct virtiodrive_s, drive);
     struct vring_virtqueue *vq = GET_GLOBAL(vdrive_g->vq);
     struct virtio_blk_outhdr hdr = {
-        .type = VIRTIO_BLK_T_IN,
+        .type = write ? VIRTIO_BLK_T_OUT : VIRTIO_BLK_T_IN,
         .ioprio = 0,
         .sector = op->lba,
     };
@@ -53,7 +53,10 @@ virtio_blk_read(struct disk_op_s *op)
     };

/* Add to virtqueue and kick host */
-    vring_add_buf(vq, sg, 1, 2, 0, 0);
+    if (write)
+        vring_add_buf(vq, sg, 2, 1, 0, 0);
+    else
+        vring_add_buf(vq, sg, 1, 2, 0, 0);
     vring_kick(GET_GLOBAL(vdrive_g->ioaddr), vq, 1);

/* Wait for reply */
@@ -78,9 +81,10 @@ process_virtio_op(struct disk_op_s *op)
         return 0;
     switch (op->command) {
     case CMD_READ:
-        return virtio_blk_read(op);
-    case CMD_FORMAT:
+        return virtio_blk_op(op, 0);
     case CMD_WRITE:
+        return virtio_blk_op(op, 1);
+    case CMD_FORMAT:
         return DISK_RET_EWRITEPROTECT;
     case CMD_RESET:
     case CMD_ISREADY:
--
    		Gleb.

[PATCHv2] add write support to virtio-blk

[Kevin O'Connor]

On Wed, Aug 18, 2010 at 05:46:20PM +0300, Gleb Natapov wrote:
> Windows XP does write to sector 0 during installation and prints
> mysterious error if write fails. Interestingly if write drops data,
> but returns OK to Windows installer installation proceed without
> complains and completes successfully.

[...]
> case CMD_FORMAT:
>     return DISK_RET_EWRITEPROTECT;

If CMD_WRITE is implemented, I think CMD_FORMAT should just return
DISK_RET_SUCCESS (as ata does).

Otherwise, looks okay to me.

-Kevin

[PATCHv2] add write support to virtio-blk

[Gleb Natapov]

On Sat, Aug 21, 2010 at 03:12:32PM -0400, Kevin O'Connor wrote:
> On Wed, Aug 18, 2010 at 05:46:20PM +0300, Gleb Natapov wrote:
> > Windows XP does write to sector 0 during installation and prints
> > mysterious error if write fails. Interestingly if write drops data,
> > but returns OK to Windows installer installation proceed without
> > complains and completes successfully.
>
> [...]
> > case CMD_FORMAT:
> >     return DISK_RET_EWRITEPROTECT;
>
> If CMD_WRITE is implemented, I think CMD_FORMAT should just return
> DISK_RET_SUCCESS (as ata does).

Can you explain why? What CMD_FORMAT is suppose to do? May be it is just
a nop for virtio disk and returning success is much better in this case.
> Otherwise, looks okay to me.
>
> -Kevin

[PATCHv2] add write support to virtio-blk

[Kevin O'Connor]

On Sat, Aug 21, 2010 at 11:02:54PM +0300, Gleb Natapov wrote:
> On Sat, Aug 21, 2010 at 03:12:32PM -0400, Kevin O'Connor wrote:
> > On Wed, Aug 18, 2010 at 05:46:20PM +0300, Gleb Natapov wrote:
> > > Windows XP does write to sector 0 during installation and prints
> > > mysterious error if write fails. Interestingly if write drops data,
> > > but returns OK to Windows installer installation proceed without
> > > complains and completes successfully.
> >
> > [...]
> > > case CMD_FORMAT:
> > >     return DISK_RET_EWRITEPROTECT;
> >
> > If CMD_WRITE is implemented, I think CMD_FORMAT should just return
> > DISK_RET_SUCCESS (as ata does).
>
> Can you explain why? What CMD_FORMAT is suppose to do? May be it is just
> a nop for virtio disk and returning success is much better in this case.

I believe format is supposed to write out the track info on floppy
disks and such.

The ata spec still has a format command in it - though I wouldn't be
surprised if modern drives didn't do anything when given that command.
(The SeaBIOS ATA code returns success on CMD_FORMAT - see
process_ata_misc_op().)

I'm not sure how calling code will react to getting a EWRITEPROTECT on
format but success on a write.  For consistency, I think it should
either succeed on both or fail on both.

I don't think we need to actually implement virtio format code - a nop
should be fine (just like what ata does).

-Kevin