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

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?

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

[Takeshi HASEGAWA]

[-- Attachment #1.1: Type: text/plain, Size: 3137 bytes --]

Thanks for the suggestions, Stefano's looks ideal and finally
it should be.

I think the change is not so urgent and afford to revise, at least while
virtio ring is not working due to the issue I posted separately.

Thanks,
Takeshi

2011/5/9 Stefano Stabellini <stefano.stabellini@eu.citrix.com>:
> 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?
>

-- 
Takeshi HASEGAWA <hasegaw@gmail.com>

[-- Attachment #1.2: Type: text/html, Size: 3848 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

_______________________________________________
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

[Wei Liu]

Refine the patch according to Stefano's advice.

1. Writing to Xenstore in libxl_device_disk_add is left as-it-is.
2. Reserved encoding for virtio disk.

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index ccf6518..6943b9d 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -1028,6 +1028,9 @@ int libxl_device_disk_add(libxl_ctx *ctx,
uint32_t domid, libxl_device_disk *dis

libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
             device.backend_kind = DEVICE_QDISK;
             break;
+        case LIBXL_DISK_BACKEND_NONE:
+	    /* Nothing to do, not a Xen VBD */
+	    break;
         default:
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk
backend type: %d\n", disk->backend);
             rc = ERROR_INVAL;
diff --git a/tools/libxl/libxl.idl b/tools/libxl/libxl.idl
index a5be66f..6b27eae 100644
--- a/tools/libxl/libxl.idl
+++ b/tools/libxl/libxl.idl
@@ -54,6 +54,7 @@ libxl_disk_backend = Enumeration("disk_backend", [
     (1, "PHY"),
     (2, "TAP"),
     (3, "QDISK"),
+    (4, "NONE"),
     ])

 libxl_nic_type = Enumeration("nic_type", [
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 5d85822..d8de1b7 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -239,6 +239,13 @@ int libxl__device_disk_dev_number(char *virtpath,
int *pdisk, int *ppartition)
         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 (2 << 28) | (disk << 8) | partition;
+    }
     return -1;
 }

diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index 76479fe..3403b5e 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -423,6 +423,10 @@ static char **
libxl__build_device_model_args_new(libxl__gc *gc,
                     drive = libxl__sprintf
                         (gc, "file=%s,if=ide,index=%d,media=disk,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,index=%d,media=disk,format=%s",
+                         disks[i].pdev_path, disk, format);
                 else
                     continue; /* Do not emulate this disk */
             }
@@ -976,6 +980,7 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,

             case LIBXL_DISK_BACKEND_PHY:
             case LIBXL_DISK_BACKEND_UNKNOWN:
+	    case LIBXL_DISK_BACKEND_NONE:
                 break;
             }
         }


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

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

[Stefano Stabellini]

On Tue, 10 May 2011, Wei Liu wrote:
> Refine the patch according to Stefano's advice.
> 
> 1. Writing to Xenstore in libxl_device_disk_add is left as-it-is.
> 2. Reserved encoding for virtio disk.
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index ccf6518..6943b9d 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1028,6 +1028,9 @@ int libxl_device_disk_add(libxl_ctx *ctx,
> uint32_t domid, libxl_device_disk *dis
> 
> libxl__device_disk_string_of_format(disk->format), disk->pdev_path));
>              device.backend_kind = DEVICE_QDISK;
>              break;
> +        case LIBXL_DISK_BACKEND_NONE:
> +	    /* Nothing to do, not a Xen VBD */
> +	    break;
>          default:
>              LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "unrecognized disk
> backend type: %d\n", disk->backend);
>              rc = ERROR_INVAL;
> diff --git a/tools/libxl/libxl.idl b/tools/libxl/libxl.idl
> index a5be66f..6b27eae 100644
> --- a/tools/libxl/libxl.idl
> +++ b/tools/libxl/libxl.idl
> @@ -54,6 +54,7 @@ libxl_disk_backend = Enumeration("disk_backend", [
>      (1, "PHY"),
>      (2, "TAP"),
>      (3, "QDISK"),
> +    (4, "NONE"),
>      ])
> 
>  libxl_nic_type = Enumeration("nic_type", [
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index 5d85822..d8de1b7 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -239,6 +239,13 @@ int libxl__device_disk_dev_number(char *virtpath,
> int *pdisk, int *ppartition)
>          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 (2 << 28) | (disk << 8) | partition;
> +    }
>      return -1;
>  }

libxl__device_disk_dev_number needs to return an error here, possibily
different from -1 to identify that the disk is not a xen VBD.
The caller needs to check for errors and return in case the disk doesn't
need a xen backend setup.


> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 76479fe..3403b5e 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -423,6 +423,10 @@ static char **
> libxl__build_device_model_args_new(libxl__gc *gc,
>                      drive = libxl__sprintf
>                          (gc, "file=%s,if=ide,index=%d,media=disk,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,index=%d,media=disk,format=%s",
> +                         disks[i].pdev_path, disk, format);
>                  else
>                      continue; /* Do not emulate this disk */
>              }
> @@ -976,6 +980,7 @@ int libxl__need_xenpv_qemu(libxl__gc *gc,
> 
>              case LIBXL_DISK_BACKEND_PHY:
>              case LIBXL_DISK_BACKEND_UNKNOWN:
> +	    case LIBXL_DISK_BACKEND_NONE:
>                  break;
>              }
>          }
> 

the rest of the patch looks fine (apart from few coding style issues)
but IanJ is quite close to have his refactoring ready so I am not sure
if he wants to accept this patch right now...
right now.