Re: [Xen-devel] [PATCH] libxl: Add AHCI support for upstream qemu

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 22 Jun 2015, Fabio Fantoni wrote:
> Usage:
> ahci=0|1 (default=0)
> 
> If enabled adds ich9 disk controller in ahci mode and uses it with
> upstream qemu to emulate disks instead of ide.
> It doesn't support cdroms which still using ide (cdroms will use
> "-device ide-cd" as new qemu parameter)
> Ahci requires new qemu parameter but for now other emulated disks cases
> remains with old ones (I did it in other patch, not needed by this one)
> I did it as libxl parameter disabled by default to avoid possible
> problems:
> - with save/restore/migration (restoring with ahci a domU that was with
> ide instead)
> - windows < 8 without pv drivers (a registry key change is needed for
> AHCI<->IDE change FWIK to avoid possible blue screen)
> - windows XP or older that many not support ahci by default.
> Setting AHCI with libxl parameter and default to disabled seems the best
> solution.
> AHCI increase hvm domUs boot performance. On linux hvm domU I saw up to
> only 20% of the previous total boot time, whereas boot time decrease a
> lot on W7 domUs for most of boots I have done. Small difference in boot
> time compared to ide mode on W8 and newer (probably other xen
> improvements or fixes are needed not ahci related)
> 
> Signed-off-by: Fabio Fantoni <fabio.fantoni@xxxxxxx>

I am OK with this patch going in Xen 4.6.


>  docs/man/xl.cfg.pod.5       |  9 +++++++++
>  tools/libxl/libxl.h         | 10 ++++++++++
>  tools/libxl/libxl_create.c  |  1 +
>  tools/libxl/libxl_dm.c      | 10 +++++++++-
>  tools/libxl/libxl_types.idl |  1 +
>  tools/libxl/xl_cmdimpl.c    |  1 +
>  6 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index a3e0e2e..7e16123 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -904,6 +904,15 @@ default is B<cd>.
>  
>  =back
>  
> +=item B<ahci=[0|1]>
> +
> +If enabled adds ich9 disk controller in ahci mode and uses it with
> +upstream qemu to emulate disks instead of ide. It decrease boot time but
> +may be not supported by default in windows xp and older windows.
> +The default is disabled (0).
> +
> +=back
> +
>  =head3 Paging
>  
>  The following options control the mechanisms used to virtualise guest
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 0a7913b..6a3677d 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -596,6 +596,16 @@ typedef struct libxl__ctx libxl_ctx;
>  #define LIBXL_HAVE_SPICE_STREAMINGVIDEO 1
>  
>  /*
> + * LIBXL_HAVE_AHCI
> + *
> + * If defined, then the u.hvm structure will contain a boolean type:
> + * ahci. This value defines if ahci support is present.
> + *
> + * If this is not defined, the ahci support is ignored.
> + */
> +#define LIBXL_HAVE_AHCI 1
> +
> +/*
>   * LIBXL_HAVE_DOMAIN_CREATE_RESTORE_PARAMS 1
>   *
>   * If this is defined, libxl_domain_create_restore()'s API has changed to
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 86384d2..8ca2481 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -331,6 +331,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          libxl_defbool_setdefault(&b_info->u.hvm.nested_hvm,         false);
>          libxl_defbool_setdefault(&b_info->u.hvm.usb,                false);
>          libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci,   true);
> +        libxl_defbool_setdefault(&b_info->u.hvm.ahci,               false);
>  
>          libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
>          if (!libxl_defbool_val(b_info->u.hvm.spice.enable) &&
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 33f9ce6..93f191a 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -818,6 +818,8 @@ static int libxl__build_device_model_args_new(libxl__gc 
> *gc,
>      flexarray_append(dm_args, libxl__sprintf(gc, "%"PRId64, ram_size));
>  
>      if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
> +        if (libxl_defbool_val(b_info->u.hvm.ahci))
> +            flexarray_append_pair(dm_args, "-device", "ahci,id=ahci0");
>          for (i = 0; i < num_disks; i++) {
>              int disk, part;
>              int dev_number =
> @@ -872,7 +874,13 @@ static int libxl__build_device_model_args_new(libxl__gc 
> *gc,
>                      drive = libxl__sprintf
>                          (gc, 
> "file=%s,if=scsi,bus=0,unit=%d,format=%s,cache=writeback",
>                           pdev_path, disk, format);
> -                else if (disk < 4)
> +                else if (disk < 6 && libxl_defbool_val(b_info->u.hvm.ahci)){

One space missing.

This means that we can now have disks up to hdf. I think you should
update the in-code comment above. I think you should also update
docs/txt/misc/vbd-interface.txt, explaining that hd* mean AHCI if ahci=1
is specified.


> +                    flexarray_vappend(dm_args, "-drive",
> +                        
> GCSPRINTF("file=%s,if=none,id=ahcidisk-%d,format=%s,cache=writeback",
> +                        pdev_path, disk, format), "-device", 
> GCSPRINTF("ide-hd,bus=ahci0.%d,unit=0,drive=ahcidisk-%d",
> +                        disk, disk), NULL);
> +                    continue;

It might be worth moving:

    flexarray_append(dm_args, "-drive");
    flexarray_append(dm_args, drive);

up from below to the multiple if statement here for uniformity.



> +                }else if (disk < 4)

one space missing


>                      drive = libxl__sprintf
>                          (gc, 
> "file=%s,if=ide,index=%d,media=disk,format=%s,cache=writeback",
>                           pdev_path, disk, format);
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 23f27d4..f107689 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -439,6 +439,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("nested_hvm",       libxl_defbool),
>                                         ("smbios_firmware",  string),
>                                         ("acpi_firmware",    string),
> +                                       ("ahci",             libxl_defbool),
>                                         ("nographic",        libxl_defbool),
>                                         ("vga",              
> libxl_vga_interface_info),
>                                         ("vnc",              libxl_vnc_info),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index c858068..74b473e 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -2216,6 +2216,7 @@ skip_vfb:
>          xlu_cfg_replace_string (config, "soundhw", &b_info->u.hvm.soundhw, 
> 0);
>          xlu_cfg_get_defbool(config, "xen_platform_pci",
>                              &b_info->u.hvm.xen_platform_pci, 0);
> +        xlu_cfg_get_defbool(config, "ahci", &b_info->u.hvm.ahci, 0);
>  
>          if(b_info->u.hvm.vnc.listen
>             && b_info->u.hvm.vnc.display
> -- 
> 1.9.1
> 

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