Re: [RFC PATCH 08/12] libxl: Q35 support (new option device_model_machine)

["Roger Pau Monné" <roger.pau@citrix.com>]

On Tue, Mar 13, 2018 at 04:33:53AM +1000, Alexey Gerasimenko wrote:
> Provide a new domain config option to select the emulated machine type,
> device_model_machine. It has following possible values:
> - "i440" - i440 emulation (default)
> - "q35" - emulate a Q35 machine. By default, the storage interface is AHCI.

I would rather name this machine_chipset or device_model_chipset.

> 
> Note that omitting device_model_machine parameter means i440 system
> by default, so the default behavior doesn't change for existing domain
> config files.
> 
> Setting device_model_machine to "q35" sends '-machine q35,accel=xen'
> argument to QEMU. Unlike i440, there no separate machine type
> to enable/disable Xen platform device, it is controlled via a machine

But I assume the xen_platform_pci option still works as expected?

> property only. See 'libxl: Xen Platform device support for Q35' patch for
> a detailed description.
> 
> Signed-off-by: Alexey Gerasimenko <x1917x@gmail.com>
> ---
>  tools/libxl/libxl_dm.c      | 16 ++++++++++------
>  tools/libxl/libxl_types.idl |  7 +++++++
>  tools/xl/xl_parse.c         | 14 ++++++++++++++
>  3 files changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index a3cddce8b7..7b531050c7 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -1443,13 +1443,17 @@ static int libxl__build_device_model_args_new(libxl__gc *gc,
>              flexarray_append(dm_args, b_info->extra_pv[i]);
>          break;
>      case LIBXL_DOMAIN_TYPE_HVM:
> -        if (!libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) {
> -            /* Switching here to the machine "pc" which does not add
> -             * the xen-platform device instead of the default "xenfv" machine.
> -             */
> -            machinearg = libxl__strdup(gc, "pc,accel=xen");
> +        if (b_info->device_model_machine == LIBXL_DEVICE_MODEL_MACHINE_Q35) {
> +            machinearg = libxl__sprintf(gc, "q35,accel=xen");
>          } else {
> -            machinearg = libxl__strdup(gc, "xenfv");
> +            if (!libxl_defbool_val(b_info->u.hvm.xen_platform_pci)) {
> +                /* Switching here to the machine "pc" which does not add
> +                 * the xen-platform device instead of the default "xenfv" machine.
> +                 */
> +                machinearg = libxl__strdup(gc, "pc,accel=xen");
> +            } else {
> +                machinearg = libxl__strdup(gc, "xenfv");
> +            }
>          }
>          if (b_info->u.hvm.mmio_hole_memkb) {
>              uint64_t max_ram_below_4g = (1ULL << 32) -
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 35038120ca..f3ef3cbdde 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -101,6 +101,12 @@ libxl_device_model_version = Enumeration("device_model_version", [
>      (2, "QEMU_XEN"),             # Upstream based qemu-xen device model
>      ])
>  
> +libxl_device_model_machine = Enumeration("device_model_machine", [
> +    (0, "UNKNOWN"),

Shouldn't this be named DEFAULT?

> +    (1, "I440"),
> +    (2, "Q35"),
> +    ])
> +
>  libxl_console_type = Enumeration("console_type", [
>      (0, "UNKNOWN"),
>      (1, "SERIAL"),
> @@ -491,6 +497,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>      ("device_model_ssid_label", string),
>      # device_model_user is not ready for use yet
>      ("device_model_user", string),
> +    ("device_model_machine", libxl_device_model_machine),
>  
>      # extra parameters pass directly to qemu, NULL terminated
>      ("extra",            libxl_string_list),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index f6842540ca..a7506a426b 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2110,6 +2110,20 @@ skip_usbdev:
>      xlu_cfg_replace_string(config, "device_model_user",
>                             &b_info->device_model_user, 0);
>  
> +    if (!xlu_cfg_get_string (config, "device_model_machine", &buf, 0)) {
> +        if (!strcmp(buf, "i440")) {
> +            b_info->device_model_machine = LIBXL_DEVICE_MODEL_MACHINE_I440;
> +        } else if (!strcmp(buf, "q35")) {
> +            b_info->device_model_machine = LIBXL_DEVICE_MODEL_MACHINE_Q35;
> +        } else {
> +            fprintf(stderr,
> +                    "Unknown device_model_machine \"%s\" specified\n", buf);
> +            exit(1);
> +        }
> +    } else {
> +        b_info->device_model_machine = LIBXL_DEVICE_MODEL_MACHINE_UNKNOWN;

That seems to be it's usage. I'm not sure you should explicitly set it
in the default case (DEFAULT == 0 already).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel