[PATCH v2] libxl: Add AHCI support for upstream qemu

[PATCH v2] libxl: Add AHCI support for upstream qemu

[Fabio Fantoni]

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@m2r.biz>

---

Changes in v2:
- libxl_dm.c: small code style fix
- added vbd-interface.txt changes
---
 docs/man/xl.cfg.pod.5       |  9 +++++++++
 docs/misc/vbd-interface.txt |  5 +++--
 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 +
 7 files changed, 34 insertions(+), 3 deletions(-)

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/docs/misc/vbd-interface.txt b/docs/misc/vbd-interface.txt
index f873db0..afb6846 100644
--- a/docs/misc/vbd-interface.txt
+++ b/docs/misc/vbd-interface.txt
@@ -3,18 +3,19 @@ Xen guest interface
 
 A Xen guest can be provided with block devices.  These are always
 provided as Xen VBDs; for HVM guests they may also be provided as
-emulated IDE or SCSI disks.
+emulated IDE, AHCI or SCSI disks.
 
 The abstract interface involves specifying, for each block device:
 
  * Nominal disk type: Xen virtual disk (aka xvd*, the default); SCSI
-   (sd*); IDE (hd*).
+   (sd*); IDE or AHCI (hd*).
 
    For HVM guests, each whole-disk hd* and and sd* device is made
    available _both_ via emulated IDE resp. SCSI controller, _and_ as a
    Xen VBD.  The HVM guest is entitled to assume that the IDE or SCSI
    disks available via the emulated IDE controller target the same
    underlying devices as the corresponding Xen VBD (ie, multipath).
+   In hd* case with ahci=1, disk will be AHCI via emulated ich9 controller.
 
    For PV guests every device is made available to the guest only as a
    Xen VBD.  For these domains the type is advisory, for use by the
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..9216028 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)) {
+                    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;
+                } else if (disk < 4)
                     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

Re: [PATCH v2] libxl: Add AHCI support for upstream qemu

[Stefano Stabellini]

On Tue, 23 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@m2r.biz>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


> 
> Changes in v2:
> - libxl_dm.c: small code style fix
> - added vbd-interface.txt changes
> ---
>  docs/man/xl.cfg.pod.5       |  9 +++++++++
>  docs/misc/vbd-interface.txt |  5 +++--
>  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 +
>  7 files changed, 34 insertions(+), 3 deletions(-)
> 
> 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/docs/misc/vbd-interface.txt b/docs/misc/vbd-interface.txt
> index f873db0..afb6846 100644
> --- a/docs/misc/vbd-interface.txt
> +++ b/docs/misc/vbd-interface.txt
> @@ -3,18 +3,19 @@ Xen guest interface
>  
>  A Xen guest can be provided with block devices.  These are always
>  provided as Xen VBDs; for HVM guests they may also be provided as
> -emulated IDE or SCSI disks.
> +emulated IDE, AHCI or SCSI disks.
>  
>  The abstract interface involves specifying, for each block device:
>  
>   * Nominal disk type: Xen virtual disk (aka xvd*, the default); SCSI
> -   (sd*); IDE (hd*).
> +   (sd*); IDE or AHCI (hd*).
>  
>     For HVM guests, each whole-disk hd* and and sd* device is made
>     available _both_ via emulated IDE resp. SCSI controller, _and_ as a
>     Xen VBD.  The HVM guest is entitled to assume that the IDE or SCSI
>     disks available via the emulated IDE controller target the same
>     underlying devices as the corresponding Xen VBD (ie, multipath).
> +   In hd* case with ahci=1, disk will be AHCI via emulated ich9 controller.
>  
>     For PV guests every device is made available to the guest only as a
>     Xen VBD.  For these domains the type is advisory, for use by the
> 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..9216028 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)) {
> +                    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;
> +                } else if (disk < 4)
>                      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
> 

Re: [PATCH v2] libxl: Add AHCI support for upstream qemu

[Ian Campbell]

On Tue, 2015-06-23 at 11:15 +0200, Fabio Fantoni wrote:
> Usage:
> ahci=0|1 (default=0)

I think a global rather than per disk option is OK (I can't think why a
user would want to mix and match) but maybe we should consider using an
enum (with values ide and ahci, defaulting to ide in libxl) so that we
can add support for whatever fancy new disk controller everyone is using
in 5 years time?

> 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)

I don't follow this reference to "will use" and a new qemu parameter,
there seems to be nothing corresponding in this patch AFAICT.

> 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)

You can drop the reference to the other patch I think.

> 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)

What is "FWIK"?

> - 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@m2r.biz>
> 
> ---
> 
> Changes in v2:
> - libxl_dm.c: small code style fix
> - added vbd-interface.txt changes
> ---
>  docs/man/xl.cfg.pod.5       |  9 +++++++++
>  docs/misc/vbd-interface.txt |  5 +++--
>  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 +
>  7 files changed, 34 insertions(+), 3 deletions(-)
> 
> 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]>

"=item B<ahci=BOOLEAN>" please.

> +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

"decreases"

> +may be not supported by default in windows xp and older windows.
> +The default is disabled (0).

"may not be supported".

I think AHCI and IDE should be capitalised in the text (not the option
name). As should "Windows XP" and "Windows"

> +
> +=back
> +
>  =head3 Paging
>  
>  The following options control the mechanisms used to virtualise guest
> diff --git a/docs/misc/vbd-interface.txt b/docs/misc/vbd-interface.txt
> index f873db0..afb6846 100644
> --- a/docs/misc/vbd-interface.txt
> +++ b/docs/misc/vbd-interface.txt
> @@ -3,18 +3,19 @@ Xen guest interface
>  
>  A Xen guest can be provided with block devices.  These are always
>  provided as Xen VBDs; for HVM guests they may also be provided as
> -emulated IDE or SCSI disks.
> +emulated IDE, AHCI or SCSI disks.
>  
>  The abstract interface involves specifying, for each block device:
>  
>   * Nominal disk type: Xen virtual disk (aka xvd*, the default); SCSI
> -   (sd*); IDE (hd*).
> +   (sd*); IDE or AHCI (hd*).
>  
>     For HVM guests, each whole-disk hd* and and sd* device is made
>     available _both_ via emulated IDE resp. SCSI controller, _and_ as a
>     Xen VBD.  The HVM guest is entitled to assume that the IDE or SCSI
>     disks available via the emulated IDE controller target the same
>     underlying devices as the corresponding Xen VBD (ie, multipath).
> +   In hd* case with ahci=1, disk will be AHCI via emulated ich9 controller.
>  
>     For PV guests every device is made available to the guest only as a
>     Xen VBD.  For these domains the type is advisory, for use by the
> 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..9216028 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");

Do we still want this even if num_disks == 0?

>          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)) {
> +                    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",

This line looks to be overlong, and in general the wrapping in this new
code seems rather arbitrary. 

How about:
                    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;

>From a flow of control PoV this continue is a little jarring.

How about adding a new const char *device = NULL to complement the
existing drive and doing the add in the common bit with an if (device)?

Re: [PATCH v2] libxl: Add AHCI support for upstream qemu

[Fabio Fantoni]

Il 25/06/2015 12:21, Ian Campbell ha scritto:
> On Tue, 2015-06-23 at 11:15 +0200, Fabio Fantoni wrote:
>> Usage:
>> ahci=0|1 (default=0)
> I think a global rather than per disk option is OK (I can't think why a
> user would want to mix and match) but maybe we should consider using an
> enum (with values ide and ahci, defaulting to ide in libxl) so that we
> can add support for whatever fancy new disk controller everyone is using
> in 5 years time?

ahci was added 4 years ago in qemu and I don't know of newer similar 
tecnology, in the case of enum probably shold be more generic for 
include more future possibility or I'm wrong? in that case what can be 
the name?
@stabellini and other developer: any advice about this?

>
>> 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)
> I don't follow this reference to "will use" and a new qemu parameter,
> there seems to be nothing corresponding in this patch AFAICT.

Is a "FAQ" for explain why cdrom is still ide and ahci use "new" qemu 
parameters (added in qemu 5 or 6 years ago but still not fully used in 
xen... :( )

>
>> 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)
> You can drop the reference to the other patch I think.

I thinked also in this case that can be useful as "FAQ".

>
>> 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)
> What is "FWIK"?

FWIK= for what I know
on physical computer I see that change ide<->ahci in bios without change 
in windows do a blue screen on boot, on xen domUs with pv driver don't 
do but without probably will do the same. In any case is not a xen problem.

>
>> - 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@m2r.biz>
>>
>> ---
>>
>> Changes in v2:
>> - libxl_dm.c: small code style fix
>> - added vbd-interface.txt changes
>> ---
>>   docs/man/xl.cfg.pod.5       |  9 +++++++++
>>   docs/misc/vbd-interface.txt |  5 +++--
>>   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 +
>>   7 files changed, 34 insertions(+), 3 deletions(-)
>>
>> 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]>
> "=item B<ahci=BOOLEAN>" please.

I'll do in v3

>
>> +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
> "decreases"

I'll do in v3

>
>> +may be not supported by default in windows xp and older windows.
>> +The default is disabled (0).
> "may not be supported".
>
> I think AHCI and IDE should be capitalised in the text (not the option
> name). As should "Windows XP" and "Windows"

I'll do in v3

>
>> +
>> +=back
>> +
>>   =head3 Paging
>>   
>>   The following options control the mechanisms used to virtualise guest
>> diff --git a/docs/misc/vbd-interface.txt b/docs/misc/vbd-interface.txt
>> index f873db0..afb6846 100644
>> --- a/docs/misc/vbd-interface.txt
>> +++ b/docs/misc/vbd-interface.txt
>> @@ -3,18 +3,19 @@ Xen guest interface
>>   
>>   A Xen guest can be provided with block devices.  These are always
>>   provided as Xen VBDs; for HVM guests they may also be provided as
>> -emulated IDE or SCSI disks.
>> +emulated IDE, AHCI or SCSI disks.
>>   
>>   The abstract interface involves specifying, for each block device:
>>   
>>    * Nominal disk type: Xen virtual disk (aka xvd*, the default); SCSI
>> -   (sd*); IDE (hd*).
>> +   (sd*); IDE or AHCI (hd*).
>>   
>>      For HVM guests, each whole-disk hd* and and sd* device is made
>>      available _both_ via emulated IDE resp. SCSI controller, _and_ as a
>>      Xen VBD.  The HVM guest is entitled to assume that the IDE or SCSI
>>      disks available via the emulated IDE controller target the same
>>      underlying devices as the corresponding Xen VBD (ie, multipath).
>> +   In hd* case with ahci=1, disk will be AHCI via emulated ich9 controller.
>>   
>>      For PV guests every device is made available to the guest only as a
>>      Xen VBD.  For these domains the type is advisory, for use by the
>> 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..9216028 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");
> Do we still want this even if num_disks == 0?

Is there no difference about disk hotplugged but ahci is rilevant only 
at boot time? If yes I'll add the if.

>
>>           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)) {
>> +                    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",
> This line looks to be overlong, and in general the wrapping in this new
> code seems rather arbitrary.
>
> How about:
>                      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);
> ?

I'll do in v3

>
>> +                    continue;
>  From a flow of control PoV this continue is a little jarring.
>
> How about adding a new const char *device = NULL to complement the
> existing drive and doing the add in the common bit with an if (device)?
>
>

the 2 continue cases in emulated disks will be removed on my other patch 
about new qemu parameter for disks and cdrom, I must change the thing 
also here? I also not understand exatcly what you mean in this change.

Re: [PATCH v2] libxl: Add AHCI support for upstream qemu

[Stefano Stabellini]

On Thu, 25 Jun 2015, Fabio Fantoni wrote:
> Il 25/06/2015 12:21, Ian Campbell ha scritto:
> > On Tue, 2015-06-23 at 11:15 +0200, Fabio Fantoni wrote:
> > > Usage:
> > > ahci=0|1 (default=0)
> > I think a global rather than per disk option is OK (I can't think why a
> > user would want to mix and match) but maybe we should consider using an
> > enum (with values ide and ahci, defaulting to ide in libxl) so that we
> > can add support for whatever fancy new disk controller everyone is using
> > in 5 years time?
> 
> ahci was added 4 years ago in qemu and I don't know of newer similar
> tecnology, in the case of enum probably shold be more generic for include more
> future possibility or I'm wrong? in that case what can be the name?
> @stabellini and other developer: any advice about this?

I don't know of any other block technologies that would use "hd" as
block device names. Virtio-blk uses "vd", so it couldn't be confused.
However for the sake of being future proof, it might make sense to
introduce an enum, maybe something like "hdtype"?

enum hdtype {
    ide,
    ahci,
}

then in the config file:

hdtype=ahci

Re: [PATCH v2] libxl: Add AHCI support for upstream qemu

[Malcolm Crossley]

On 25/06/15 12:15, Fabio Fantoni wrote:
> Il 25/06/2015 12:21, Ian Campbell ha scritto:
>> On Tue, 2015-06-23 at 11:15 +0200, Fabio Fantoni wrote:
>>> Usage:
>>> ahci=0|1 (default=0)
>> I think a global rather than per disk option is OK (I can't think why a
>> user would want to mix and match) but maybe we should consider using an
>> enum (with values ide and ahci, defaulting to ide in libxl) so that we
>> can add support for whatever fancy new disk controller everyone is using
>> in 5 years time?
> 
> ahci was added 4 years ago in qemu and I don't know of newer similar tecnology, in the case of enum
> probably shold be more generic for include more future possibility or I'm wrong? in that case what
> can be the name?
> @stabellini and other developer: any advice about this?

You may want to support nvme device interface as well. This would be the newer similar technology
you are referring to :)

Re: [PATCH v2] libxl: Add AHCI support for upstream qemu

[Ian Campbell]

On Thu, 2015-06-25 at 12:58 +0100, Stefano Stabellini wrote:
> On Thu, 25 Jun 2015, Fabio Fantoni wrote:
> > Il 25/06/2015 12:21, Ian Campbell ha scritto:
> > > On Tue, 2015-06-23 at 11:15 +0200, Fabio Fantoni wrote:
> > > > Usage:
> > > > ahci=0|1 (default=0)
> > > I think a global rather than per disk option is OK (I can't think why a
> > > user would want to mix and match) but maybe we should consider using an
> > > enum (with values ide and ahci, defaulting to ide in libxl) so that we
> > > can add support for whatever fancy new disk controller everyone is using
> > > in 5 years time?
> > 
> > ahci was added 4 years ago in qemu and I don't know of newer similar
> > tecnology,

It maybe hasn't been invented yet (although Malcolm suggested nvme,
which is plausible).

>  in the case of enum probably shold be more generic for include more
> > future possibility or I'm wrong? in that case what can be the name?
> > @stabellini and other developer: any advice about this?
> 
> I don't know of any other block technologies that would use "hd" as
> block device names. Virtio-blk uses "vd", so it couldn't be confused.
> However for the sake of being future proof, it might make sense to
> introduce an enum, maybe something like "hdtype"?
> 
> enum hdtype {
>     ide,
>     ahci,
> }
> 
> then in the config file:
> 
> hdtype=ahci

That's the sort of thing I was imagining, yes.

Ian.

Re: [PATCH v2] libxl: Add AHCI support for upstream qemu

[Fabio Fantoni]

Il 25/06/2015 14:18, Ian Campbell ha scritto:
> On Thu, 2015-06-25 at 12:58 +0100, Stefano Stabellini wrote:
>> On Thu, 25 Jun 2015, Fabio Fantoni wrote:
>>> Il 25/06/2015 12:21, Ian Campbell ha scritto:
>>>> On Tue, 2015-06-23 at 11:15 +0200, Fabio Fantoni wrote:
>>>>> Usage:
>>>>> ahci=0|1 (default=0)
>>>> I think a global rather than per disk option is OK (I can't think why a
>>>> user would want to mix and match) but maybe we should consider using an
>>>> enum (with values ide and ahci, defaulting to ide in libxl) so that we
>>>> can add support for whatever fancy new disk controller everyone is using
>>>> in 5 years time?
>>> ahci was added 4 years ago in qemu and I don't know of newer similar
>>> tecnology,
> It maybe hasn't been invented yet (although Malcolm suggested nvme,
> which is plausible).
>
>>   in the case of enum probably shold be more generic for include more
>>> future possibility or I'm wrong? in that case what can be the name?
>>> @stabellini and other developer: any advice about this?
>> I don't know of any other block technologies that would use "hd" as
>> block device names. Virtio-blk uses "vd", so it couldn't be confused.
>> However for the sake of being future proof, it might make sense to
>> introduce an enum, maybe something like "hdtype"?
>>
>> enum hdtype {
>>      ide,
>>      ahci,
>> }
>>
>> then in the config file:
>>
>> hdtype=ahci
> That's the sort of thing I was imagining, yes.
>
> Ian.
>

Good, I'll do now in v3