On Wed, Jul 24, 2024 at 7:50 AM Markus Armbruster <arm...@redhat.com> wrote:
>
> Fabiano Rosas <faro...@suse.de> writes:
>
> > Yichen Wang <yichen.w...@bytedance.com> writes:
> >
> >> On Thu, Jul 11, 2024 at 2:53 PM Yichen Wang <yichen.w...@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().
>
I tried Fabiano's suggestion, and put a unit32_t in MigrateState data
structure. I got exactly the same: "qemu-system-x86_64.dsa:
../../../qapi/string-input-visitor.c:343: parse_type_str: Assertion
`siv->lm == LM_NONE' failed.". Steve's patch is more to be a read-only
field from HMP, so probably I can't do that. Markus's suggestion seems
to be too heavy for the patch and I took a quick glance and it doesn't
seem to be easy to do.
So should we revert to the old "str" format instead of strList? Or how
should I proceed here?
