Re: [Xen-devel] [PATCH libvirt v2 2/2] libxl: support vscsi

[Jim Fehlig <jfehlig@xxxxxxxx>]

On 04/13/2016 03:15 AM, Olaf Hering wrote:
> This uses the API version of the proposed vscsi support in libxl (v12):
> http://lists.xenproject.org/archives/html/xen-devel/2016-04/msg01772.html

We'll need to wait for that to land in xen.git before committing libxl scsi
support to libvirt.git, but thanks for putting the current work up for review.

> Is there anything else that needs to be done in libvirt?

We'll need code in src/xenconfig/xen_xl.* to convert xl vscis cfg <-> libvirt
domXML, along with tests in tests/xlconfigtest.

>  Right now libvirt scsi
> support is very simple minded, no support at all to describe host devices with
> persistant names.

Yes, I think you are correct, but I'm not aware of any reason that support
couldn't be added.

>  Example used during testing:
>
>  <hostdev mode='subsystem' type='scsi' managed='no' sgio='filtered' 
> rawio='yes'>

IIRC, the 'managed' attribute only applies to PCI hostdevs.

>   <source>
>    <adapter name='scsi_host5'/>
>    <address bus='0' target='1' unit='0'/>
>   </source>
>   <readonly/>
>   <address type='drive' controller='0' bus='0' target='0' unit='0'/>
>  </hostdev>
>
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> Cc: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
>  src/libxl/libxl_conf.c   |  59 +++++++++++++++
>  src/libxl/libxl_conf.h   |   1 +
>  src/libxl/libxl_domain.c |   2 +-
>  src/libxl/libxl_driver.c | 187 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 248 insertions(+), 1 deletion(-)

Needs rebased against current libvirt.git master.

>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index f5ef50f..1e3615e 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1915,6 +1915,61 @@ libxlMakePCIList(virDomainDefPtr def, 
> libxl_domain_config *d_config)
>  }
>  
>  static int
> +libxlMakeVscsiList(libxl_ctx *ctx,
> +                   XLU_Config *xlu,
> +                   virDomainDefPtr def,
> +                   libxl_domain_config *d_config)
> +{
> +    virDomainHostdevDefPtr *l_hostdevs = def->hostdevs;
> +    size_t i, nhostdevs = def->nhostdevs;
> +    virDomainHostdevDefPtr hostdev;
> +    virDomainHostdevSubsysSCSIPtr scsisrc;
> +    char *str;
> +    int rc = 0;
> +
> +    if (nhostdevs == 0)
> +        return 0;
> +
> +    for (i = 0; i < nhostdevs; i++) {
> +        hostdev = l_hostdevs[i];
> +        if (hostdev->mode != VIR_DOMAIN_HOSTDEV_MODE_SUBSYS)
> +            continue;
> +        if (hostdev->source.subsys.type != 
> VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI)
> +            continue;
> +        scsisrc = &hostdev->source.subsys.u.scsi;
> +        if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
> +            continue;
> +#if defined(LIBXL_HAVE_VSCSI)
> +        if (virAsprintf(&str, "%s:%u:%u:%u,%u:%u:%u:%llu%s",
> +                     scsisrc->u.host.adapter + strlen("scsi_host"),
> +                     scsisrc->u.host.bus,
> +                     scsisrc->u.host.target,
> +                     scsisrc->u.host.unit,
> +                     hostdev->info->addr.drive.controller,
> +                     hostdev->info->addr.drive.bus,
> +                     hostdev->info->addr.drive.target,
> +                     hostdev->info->addr.drive.unit,
> +                     scsisrc->rawio == VIR_TRISTATE_BOOL_YES ? 
> ",feature-host" : "") < 0) {
> +            goto error;
> +        };
> +        rc = xlu_vscsi_config_add(xlu, ctx, str, &d_config->num_vscsictrls, 
> &d_config->vscsictrls);
> +        VIR_FREE(str);
> +        if (rc)
> +            goto error;
> +#else
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("This version of libxenlight does not support 
> vscsi"));
> +        goto error;
> +#endif
> +    }
> +
> +    return 0;
> +
> + error:
> +    return -1;
> +}
> +
> +static int
>  libxlMakeVideo(virDomainDefPtr def, libxl_domain_config *d_config)
>  
>  {
> @@ -2059,6 +2114,7 @@ int
>  libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>                         virDomainDefPtr def,
>                         libxl_ctx *ctx,
> +                       XLU_Config *xlu,
>                         libxl_domain_config *d_config)
>  {
>      libxl_domain_config_init(d_config);
> @@ -2084,6 +2140,9 @@ libxlBuildDomainConfig(virPortAllocatorPtr 
> graphicsports,
>      if (libxlMakePCIList(def, d_config) < 0)
>          return -1;
>  
> +    if (libxlMakeVscsiList(ctx, xlu, def, d_config) < 0)
> +        return -1;
> +
>      /*
>       * Now that any potential VFBs are defined, update the build info with
>       * the data of the primary display. Some day libxl might implicitely do
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index b069e45..422c1d6 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -218,6 +218,7 @@ int
>  libxlBuildDomainConfig(virPortAllocatorPtr graphicsports,
>                         virDomainDefPtr def,
>                         libxl_ctx *ctx,
> +                       XLU_Config *xlu,
>                         libxl_domain_config *d_config);
>  
>  static inline void
> diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c
> index aed904b..02379c9 100644
> --- a/src/libxl/libxl_domain.c
> +++ b/src/libxl/libxl_domain.c
> @@ -1051,7 +1051,7 @@ libxlDomainStart(libxlDriverPrivatePtr driver, 
> virDomainObjPtr vm,
>      VIR_FREE(priv->lockState);
>  
>      if (libxlBuildDomainConfig(driver->reservedGraphicsPorts, vm->def,
> -                               cfg->ctx, &d_config) < 0)
> +                               cfg->ctx, cfg->xlu, &d_config) < 0)
>          goto cleanup_dom;
>  
>      if (cfg->autoballoon && libxlDomainFreeMem(cfg->ctx, &d_config) < 0)
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index bf97c9c..186f9d4 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -3024,6 +3024,117 @@ libxlDomainAttachHostPCIDevice(libxlDriverPrivatePtr 
> driver,
>  }
>  
>  static int
> +libxlDomainAttachHostSCSIDevice(libxlDriverPrivatePtr driver,
> +                               virDomainObjPtr vm,
> +                               virDomainHostdevDefPtr hostdev)
> +{
> +#if defined(LIBXL_HAVE_VSCSI)
> +    libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
> +    libxl_device_vscsictrl ctrl, existing;
> +    libxl_device_vscsidev dev;
> +    bool found_existing;
> +    virDomainHostdevDefPtr found;
> +    virHostdevManagerPtr hostdev_mgr = driver->hostdevMgr;
> +    virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi;
> +    char *str = NULL;
> +    int ret = -1;
> +
> +    if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI)
> +        return -1;
> +
> +    libxl_device_vscsictrl_init(&existing);
> +    libxl_device_vscsictrl_init(&ctrl);
> +    libxl_device_vscsidev_init(&dev);
> +
> +    if (virDomainHostdevFind(vm->def, hostdev, &found) >= 0) {
> +        virReportError(VIR_ERR_OPERATION_FAILED,
> +                       _("target scsi device %u:%u:%u:%llu already exists"),
> +                       hostdev->info->addr.drive.controller,
> +                       hostdev->info->addr.drive.bus,
> +                       hostdev->info->addr.drive.target,
> +                       hostdev->info->addr.drive.unit);
> +        goto cleanup;
> +    }
> +
> +    if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
> +        goto cleanup;
> +
> +    if (virHostdevPrepareSCSIDevices(hostdev_mgr, LIBXL_DRIVER_NAME,
> +                                     vm->def->name, &hostdev, 1) < 0)
> +        goto cleanup;
> +
> +    if (virAsprintf(&str, "%s:%u:%u:%u,%u:%u:%u:%llu%s",
> +                    scsisrc->u.host.adapter + strlen("scsi_host"),
> +                    scsisrc->u.host.bus,
> +                    scsisrc->u.host.target,
> +                    scsisrc->u.host.unit,
> +                    hostdev->info->addr.drive.controller,
> +                    hostdev->info->addr.drive.bus,
> +                    hostdev->info->addr.drive.target,
> +                    hostdev->info->addr.drive.unit,
> +                    scsisrc->rawio == VIR_TRISTATE_BOOL_YES ?
> +                    ",feature-host" : "") < 0) {

It feels like the tedious creation of these strings should be done by a helper
function (similar to virSCSIDeviceGetSgName) instead of repeated code.

Otherwise looking good.

Regards,
Jim


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel