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

[Alexey G <x1917x@xxxxxxxxx>]

On Mon, 19 Mar 2018 17:01:18 +0000
Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:

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

device_model_ prefix is a must I think -- multiple device model related
options have names starting with device_model_.

device_model_chipset... well, maybe, but we're actually specifying a
QEMU machine here. In QEMU mailing list there was even a suggestion
to allow to pass a machine version number here, like "pc-q35-2.10".
I think some opinions are needed here.

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

Yes, xen_platform_pci should work as before.

>> property only. See 'libxl: Xen Platform device support for Q35'
>> patch for a detailed description.
>> 
>> Signed-off-by: Alexey Gerasimenko <x1917x@xxxxxxxxx>
>> ---
>>  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?

"Unknown" here should be read as "unspecified", but I guess DEFAULT
will be clearer anyway.

>> +    (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).

Will check this, although setting the variable value explicitly is good
for code readability I think.

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