[PATCH] libxl: virtio-blk-pci support for FV domain

[PATCH] libxl: virtio-blk-pci support for FV domain

[Takeshi HASEGAWA]

I am working to support virtio-blk-pci device on Xen FV domain.
This patch adds virtio-blk-pci support to libxl.

New vbd type (252 << 8) is added.
I borrow 252 from major number of linux virtio-blk frontend (vd).
virtio-blk-pci does not use this type right now, but it should
also work with Wei Liu's work.


I have already seen virtio device on HVM domain with upstream-qemu,
but there looks some issues when I/O occurs.
(fdisk works, but qemu dies during mkfs)
I am going to look into the problem.

Any suggestions are welcome!

Thanks,
Takeshi


Signed-off-by: Takeshi HASEGAWA <hasegaw@gmail.com>

diff -r 4b0692880dfa docs/misc/vbd-interface.txt
--- a/docs/misc/vbd-interface.txt       Thu May 05 17:40:34 2011 +0100
+++ b/docs/misc/vbd-interface.txt       Sun May 08 12:46:00 2011 +0900
@@ -73,6 +73,7 @@

     1 << 28 | disk << 8 | partition      xvd, disks or partitions 16 onwards
    202 << 8 | disk << 4 | partition      xvd, disks and partitions up to 15
+   252 << 8 | disk << 4 | partition      vd, disks and partitions up to 15
      8 << 8 | disk << 4 | partition      sd, disks and partitions up to 15
      3 << 8 | disk << 6 | partition      hd, disks 0..1, partitions 0..63
     22 << 8 | (disk-2) << 6 | partition  hd, disks 2..3, partitions 0..63
diff -r 4b0692880dfa tools/libxl/libxl_device.c
--- a/tools/libxl/libxl_device.c        Thu May 05 17:40:34 2011 +0100
+++ b/tools/libxl/libxl_device.c        Sun May 08 12:46:00 2011 +0900
@@ -238,6 +238,13 @@
         if (pdisk) *pdisk = disk;
         if (ppartition) *ppartition = partition;
         return (8 << 8) | (disk << 4) | partition;
+    }
+    if (device_virtdisk_matches(virtpath, "vd",
+                                &disk, 15,
+                                &partition, 15)) {
+        if (pdisk) *pdisk = disk;
+        if (ppartition) *ppartition = partition;
+        return (252 << 8) | (disk << 4) | partition;
     }
     return -1;
 }
diff -r 4b0692880dfa tools/libxl/libxl_dm.c
--- a/tools/libxl/libxl_dm.c    Thu May 05 17:40:34 2011 +0100
+++ b/tools/libxl/libxl_dm.c    Sun May 08 12:46:00 2011 +0900
@@ -418,6 +418,10 @@
                 if (strncmp(disks[i].vdev, "sd", 2) == 0)
                     drive = libxl__sprintf
                         (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s",
+                         disks[i].pdev_path, disk, format);
+                else if (strncmp(disks[i].vdev, "vd", 2) == 0)
+                    drive = libxl__sprintf
+                        (gc, "file=%s,if=virtio,bus=0,unit=%d,format=%s",
                          disks[i].pdev_path, disk, format);
                 else if (disk < 4)
                     drive = libxl__sprintf

-- 
Takeshi HASEGAWA <hasegaw@gmail.com>

Re: [PATCH] libxl: virtio-blk-pci support for FV domain

[Wei Liu]

On Sun, May 8, 2011 at 11:48 AM, Takeshi HASEGAWA <hasegaw@gmail.com> wrote:
> I am working to support virtio-blk-pci device on Xen FV domain.
> This patch adds virtio-blk-pci support to libxl.
>
> New vbd type (252 << 8) is added.
> I borrow 252 from major number of linux virtio-blk frontend (vd).
> virtio-blk-pci does not use this type right now, but it should
> also work with Wei Liu's work.
>
>
> I have already seen virtio device on HVM domain with upstream-qemu,
> but there looks some issues when I/O occurs.
> (fdisk works, but qemu dies during mkfs)
> I am going to look into the problem.
>
> Any suggestions are welcome!
>
> Thanks,
> Takeshi
>
>
> Signed-off-by: Takeshi HASEGAWA <hasegaw@gmail.com>
>
> diff -r 4b0692880dfa docs/misc/vbd-interface.txt
> --- a/docs/misc/vbd-interface.txt       Thu May 05 17:40:34 2011 +0100
> +++ b/docs/misc/vbd-interface.txt       Sun May 08 12:46:00 2011 +0900
> @@ -73,6 +73,7 @@
>
>     1 << 28 | disk << 8 | partition      xvd, disks or partitions 16 onwards
>    202 << 8 | disk << 4 | partition      xvd, disks and partitions up to 15
> +   252 << 8 | disk << 4 | partition      vd, disks and partitions up to 15
>      8 << 8 | disk << 4 | partition      sd, disks and partitions up to 15
>      3 << 8 | disk << 6 | partition      hd, disks 0..1, partitions 0..63
>     22 << 8 | (disk-2) << 6 | partition  hd, disks 2..3, partitions 0..63
> diff -r 4b0692880dfa tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c        Thu May 05 17:40:34 2011 +0100
> +++ b/tools/libxl/libxl_device.c        Sun May 08 12:46:00 2011 +0900
> @@ -238,6 +238,13 @@
>         if (pdisk) *pdisk = disk;
>         if (ppartition) *ppartition = partition;
>         return (8 << 8) | (disk << 4) | partition;
> +    }
> +    if (device_virtdisk_matches(virtpath, "vd",
> +                                &disk, 15,
> +                                &partition, 15)) {
> +        if (pdisk) *pdisk = disk;
> +        if (ppartition) *ppartition = partition;
> +        return (252 << 8) | (disk << 4) | partition;
>     }
>     return -1;
>  }
> diff -r 4b0692880dfa tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c    Thu May 05 17:40:34 2011 +0100
> +++ b/tools/libxl/libxl_dm.c    Sun May 08 12:46:00 2011 +0900
> @@ -418,6 +418,10 @@
>                 if (strncmp(disks[i].vdev, "sd", 2) == 0)
>                     drive = libxl__sprintf
>                         (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s",
> +                         disks[i].pdev_path, disk, format);
> +                else if (strncmp(disks[i].vdev, "vd", 2) == 0)
> +                    drive = libxl__sprintf
> +                        (gc, "file=%s,if=virtio,bus=0,unit=%d,format=%s",
>                          disks[i].pdev_path, disk, format);
>                 else if (disk < 4)
>                     drive = libxl__sprintf
>
> --
> Takeshi HASEGAWA <hasegaw@gmail.com>
>

Thanks, I just started working on VirtIO block support yesterday.

I was trying to add support to parse_disk_config() so that we can
specify "-if" option in the config file.

I'm wondering which approach is better -- configure through
device_virtdisk_matches() or parse_disk_config() .

Any suggestions?

-- 
Best regards
Wei Liu
Twitter: @iliuw
Site: http://liuw.name

Re: [PATCH] libxl: virtio-blk-pci support for FV domain

[Wei Liu]

On Sun, May 8, 2011 at 12:56 PM, Wei Liu <liuw@liuw.name> wrote:
> Thanks, I just started working on VirtIO block support yesterday.
>
> I was trying to add support to parse_disk_config() so that we can
> specify "-if" option in the config file.
>
> I'm wondering which approach is better -- configure through
> device_virtdisk_matches() or parse_disk_config() .
>
> Any suggestions?

It seems that Takeshi's approach is better.

I should follow libxl's conventions.

-- 
Best regards
Wei Liu
Twitter: @iliuw
Site: http://liuw.name

Re: [PATCH] libxl: virtio-blk-pci support for FV domain

[Takeshi HASEGAWA]

My patch threats virtio block devices with same manner with xvd, sd, and hd.
Though, I need to discuss with xen developpers whether the vbd number
assignment is reasonable or not.

I guess blktap and blkback will never handle virto block device, so assigning
(reserving) the whole range of 2 << 28 for virtio block vbd may be more better.

Actually virtio is not Xen's VBD, so vbd number is not mandatory to work.
However, I believe this approach makes virtio block manageable
in same way as xvd and others.

I am looking forward for xen developpers' opinions.


Thanks,
Takeshi

2011/05/08 14:25 "Wei Liu" <liuw@liuw.name>:
> On Sun, May 8, 2011 at 12:56 PM, Wei Liu <liuw@liuw.name> wrote:
>> Thanks, I just started working on VirtIO block support yesterday.
>>
>> I was trying to add support to parse_disk_config() so that we can
>> specify "-if" option in the config file.
>>
>> I'm wondering which approach is better -- configure through
>> device_virtdisk_matches() or parse_disk_config() .
>>
>> Any suggestions?
>
> It seems that Takeshi's approach is better.
>
> I should follow libxl's conventions.
>
> --
> Best regards
> Wei Liu
> Twitter: @iliuw
> Site: http://liuw.name

Re: [PATCH] libxl: virtio-blk-pci support for FV domain

[Ian Campbell]

On Sun, 2011-05-08 at 04:48 +0100, Takeshi HASEGAWA wrote:
> I am working to support virtio-blk-pci device on Xen FV domain.
> This patch adds virtio-blk-pci support to libxl.
> 
> New vbd type (252 << 8) is added.
> I borrow 252 from major number of linux virtio-blk frontend (vd).
> virtio-blk-pci does not use this type right now, but it should
> also work with Wei Liu's work.
> 
> 
> I have already seen virtio device on HVM domain with upstream-qemu,
> but there looks some issues when I/O occurs.
> (fdisk works, but qemu dies during mkfs)
> I am going to look into the problem.
> 
> Any suggestions are welcome!
> 
> Thanks,
> Takeshi
> 
> 
> Signed-off-by: Takeshi HASEGAWA <hasegaw@gmail.com>
> 
> diff -r 4b0692880dfa docs/misc/vbd-interface.txt
> --- a/docs/misc/vbd-interface.txt       Thu May 05 17:40:34 2011 +0100
> +++ b/docs/misc/vbd-interface.txt       Sun May 08 12:46:00 2011 +0900
> @@ -73,6 +73,7 @@
> 
>      1 << 28 | disk << 8 | partition      xvd, disks or partitions 16 onwards
>     202 << 8 | disk << 4 | partition      xvd, disks and partitions up to 15
> +   252 << 8 | disk << 4 | partition      vd, disks and partitions up to 15
>       8 << 8 | disk << 4 | partition      sd, disks and partitions up to 15
>       3 << 8 | disk << 6 | partition      hd, disks 0..1, partitions 0..63
>      22 << 8 | (disk-2) << 6 | partition  hd, disks 2..3, partitions 0..63

This file describes the the Xen PV block protocol, which is completely
orthogonal to the virtio one, so why is this needed?

> diff -r 4b0692880dfa tools/libxl/libxl_device.c
> --- a/tools/libxl/libxl_device.c        Thu May 05 17:40:34 2011 +0100
> +++ b/tools/libxl/libxl_device.c        Sun May 08 12:46:00 2011 +0900
> @@ -238,6 +238,13 @@
>          if (pdisk) *pdisk = disk;
>          if (ppartition) *ppartition = partition;
>          return (8 << 8) | (disk << 4) | partition;
> +    }
> +    if (device_virtdisk_matches(virtpath, "vd",
> +                                &disk, 15,
> +                                &partition, 15)) {
> +        if (pdisk) *pdisk = disk;
> +        if (ppartition) *ppartition = partition;
> +        return (252 << 8) | (disk << 4) | partition;
>      }
>      return -1;
>  }
> diff -r 4b0692880dfa tools/libxl/libxl_dm.c
> --- a/tools/libxl/libxl_dm.c    Thu May 05 17:40:34 2011 +0100
> +++ b/tools/libxl/libxl_dm.c    Sun May 08 12:46:00 2011 +0900
> @@ -418,6 +418,10 @@
>                  if (strncmp(disks[i].vdev, "sd", 2) == 0)
>                      drive = libxl__sprintf
>                          (gc, "file=%s,if=scsi,bus=0,unit=%d,format=%s",
> +                         disks[i].pdev_path, disk, format);
> +                else if (strncmp(disks[i].vdev, "vd", 2) == 0)
> +                    drive = libxl__sprintf
> +                        (gc, "file=%s,if=virtio,bus=0,unit=%d,format=%s",
>                           disks[i].pdev_path, disk, format);
>                  else if (disk < 4)
>                      drive = libxl__sprintf
> 

Re: [PATCH] libxl: virtio-blk-pci support for FV domain

[Ian Campbell]

On Mon, 2011-05-09 at 09:56 +0100, Ian Campbell wrote:
> On Sun, 2011-05-08 at 04:48 +0100, Takeshi HASEGAWA wrote:
[...]
> > diff -r 4b0692880dfa docs/misc/vbd-interface.txt
> > --- a/docs/misc/vbd-interface.txt       Thu May 05 17:40:34 2011 +0100
> > +++ b/docs/misc/vbd-interface.txt       Sun May 08 12:46:00 2011 +0900
> > @@ -73,6 +73,7 @@
> > 
> >      1 << 28 | disk << 8 | partition      xvd, disks or partitions 16 onwards
> >     202 << 8 | disk << 4 | partition      xvd, disks and partitions up to 15
> > +   252 << 8 | disk << 4 | partition      vd, disks and partitions up to 15
> >       8 << 8 | disk << 4 | partition      sd, disks and partitions up to 15
> >       3 << 8 | disk << 6 | partition      hd, disks 0..1, partitions 0..63
> >      22 << 8 | (disk-2) << 6 | partition  hd, disks 2..3, partitions 0..63
> 
> This file describes the the Xen PV block protocol, which is completely
> orthogonal to the virtio one, so why is this needed?

Sorry, I just saw this was discussed in the thread, I should read entire
threads before responding!

Ian.

Re: Re: [PATCH] libxl: virtio-blk-pci support for FV domain

[Ian Campbell]

On Sun, 2011-05-08 at 12:26 +0100, Takeshi HASEGAWA wrote:
> My patch threats virtio block devices with same manner with xvd, sd, and hd.
> Though, I need to discuss with xen developpers whether the vbd number
> assignment is reasonable or not.
> 
> I guess blktap and blkback will never handle virto block device, so assigning
> (reserving) the whole range of 2 << 28 for virtio block vbd may be more better.

I think eating into that space would be better than exposing yet another
Linux specific major number in our interfaces.

> Actually virtio is not Xen's VBD, so vbd number is not mandatory to work.
> However, I believe this approach makes virtio block manageable
> in same way as xvd and others.
> 
> I am looking forward for xen developpers' opinions.
> 
> 
> Thanks,
> Takeshi
> 
> 2011/05/08 14:25 "Wei Liu" <liuw@liuw.name>:
> > On Sun, May 8, 2011 at 12:56 PM, Wei Liu <liuw@liuw.name> wrote:
> >> Thanks, I just started working on VirtIO block support yesterday.
> >>
> >> I was trying to add support to parse_disk_config() so that we can
> >> specify "-if" option in the config file.
> >>
> >> I'm wondering which approach is better -- configure through
> >> device_virtdisk_matches() or parse_disk_config() .
> >>
> >> Any suggestions?
> >
> > It seems that Takeshi's approach is better.
> >
> > I should follow libxl's conventions.
> >
> > --
> > Best regards
> > Wei Liu
> > Twitter: @iliuw
> > Site: http://liuw.name
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: Re: [PATCH] libxl: virtio-blk-pci support for FV domain

[Stefano Stabellini]

On Sun, 8 May 2011, Takeshi HASEGAWA wrote:
> My patch threats virtio block devices with same manner with xvd, sd, and hd.
> Though, I need to discuss with xen developpers whether the vbd number
> assignment is reasonable or not.
> 
> I guess blktap and blkback will never handle virto block device, so assigning
> (reserving) the whole range of 2 << 28 for virtio block vbd may be more better.
> 
> Actually virtio is not Xen's VBD, so vbd number is not mandatory to work.
> However, I believe this approach makes virtio block manageable
> in same way as xvd and others.
> 

Like you say virtio is not a Xen VBD protocol so I don't think we should
add a new Xen VBD encoding.
After all we don't want any Xen VBD setup for a given virtio disk.

The user should be able to specify a virtio disk using the following
syntax:

disk = ['file:/path/to/file.raw,vd0,w']

xl should parse the syntax and allocate the corresponding
libxl_device_disk struct, with vdev = "vd0".
The problem is that we need to specify a disk backend and none of the
existing disk backends are appropriate, because they are Xen VBD disk
backends while this is a completely different backend altogether.
So I would add a new libxl_disk_backend called "NONE" that means "No Xen
VBD disk backend".
So we have a libxl_device_disk with vdev = "vd0" and backend = NONE.
Eventually libxl_device_disk_add gets called with that libxl_device_disk
as parameter; validate_virtual_disk should correctly validate the disk.
After that libxl_device_disk_add calls libxl__device_disk_dev_number
that translates the vdev into a Xen VBD devid: in this case
libxl__device_disk_dev_number should return a new error that means "No
Xen VBD for this device". libxl_device_disk_add should catch the error
and return because there is nothing to do.


At this point there are still two problems to solve:

- how to handle disk hotplug
we need to add QMP support to libxl so that libxl_device_disk_add can
use it to ask qemu to dynamically allocate a new virtio disk;

- how to handle virtio for pure PV guests
in this case virtio is going to be setup through xenstore so we actually
need to write something there. But the virtio-on-xenstore protocol
doesn't exist yet so we don't really know what is going to be written
there and by whom.


This is what I would do today to add virtio-blk support to libxl,
however IanJ intends to rewrite the disk handling API in libxl so
something might end up to be very different.
Ian, do you have any comments on this?
Considering that I wouldn't want to stall the virtio-blk on xen work and
that the patch will be rather small anyway, would you be OK with
accepting a virtio-blk patch for libxl before your refactoring?