Re: [PATCH v5 08/13] migration/multifd: Add new migration option for multifd DSA offloading.

[Markus Armbruster]

Fabiano Rosas <farosas@suse.de> writes:

> Yichen Wang <yichen.wang@bytedance.com> writes:
>
>> On Thu, Jul 11, 2024 at 2:53 PM Yichen Wang <yichen.wang@bytedance.com> 
>> wrote:
>>
>>> diff --git a/migration/options.c b/migration/options.c
>>> index 645f55003d..f839493016 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>> @@ -29,6 +29,7 @@
>>>  #include "ram.h"
>>>  #include "options.h"
>>>  #include "sysemu/kvm.h"
>>> +#include <cpuid.h>
>>>
>>>  /* Maximum migrate downtime set to 2000 seconds */
>>>  #define MAX_MIGRATE_DOWNTIME_SECONDS 2000
>>> @@ -162,6 +163,10 @@ Property migration_properties[] = {
>>>      DEFINE_PROP_ZERO_PAGE_DETECTION("zero-page-detection", MigrationState,
>>>                         parameters.zero_page_detection,
>>>                         ZERO_PAGE_DETECTION_MULTIFD),
>>> +    /* DEFINE_PROP_ARRAY("dsa-accel-path", MigrationState, x, */
>>> +    /*                    parameters.dsa_accel_path, qdev_prop_string, 
>>> char *), */
>
> This is mostly correct, I think, you just need to create a field in
> MigrationState to keep the length (instead of x). However, I found out
> just now that this only works with QMP. Let me ask for other's
> opinions...
>
>>> +    /* DEFINE_PROP_STRING("dsa-accel-path", MigrationState, */
>>> +    /*                    parameters.dsa_accel_path), */
>>>
>>>      /* Migration capabilities */
>>>      DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
>>
>> I changed the dsa-accel-path to be a ['str'], i.e. strList* in C.
>> However, I am having a hard time about how to define the proper
>> properties here. I don't know what MACRO to use and I can't find good
>> examples... Need some guidance about how to proceed. Basically I will
>> need this to pass something like '-global
>> migration.dsa-accel-path="/dev/dsa/wq0.0"' in cmdline, or
>> "migrate_set_parameter dsa-accel-path" in QEMU CLI. Don't know how to
>> pass strList there.
>>
>> Thanks very much!
>
> @Daniel, @Markus, any idea here?
>
> If I'm reading this commit[1] right, it seems we decided to disallow
> passing of arrays without JSON, which affects -global on the
> command-line and HMP.
>
> 1- b06f8b500d (qdev: Rework array properties based on list visitor,
> 2023-11-09)
>
> QMP shell:
> (QEMU) migrate-set-parameters dsa-accel-path=['a','b']
> {"return": {}}
>
> HMP:
> (qemu) migrate_set_parameter dsa-accel-path "['a','b']"
> qemu-system-x86_64: ../qapi/string-input-visitor.c:343: parse_type_str:
> Assertion `siv->lm == LM_NONE' failed.

HMP migrate_set_parameter doesn't support JSON.  It uses the string
input visitor to parse the value, which can only do lists of integers.

The string visitors have been thorns in my side since forever.

> Any recommendation? I believe all migration parameters so far can be set
> via those means, I don't think we can allow only this one to be
> QMP-only.
>
> Or am I just missing something?

I don't think the string input visitor can be compatibly extended to
arbitrary lists.

We could replace HMP migrate_set_parameter by migrate_set_parameters.
The new command parses its single argument into a struct
MigrateSetParameters with keyval_parse(),
qobject_input_visitor_new_keyval(), and
visit_type_MigrateSetParameters().