Re: [qemu PATCH] docs: fix missing backslash in certtool shell example

[Peter Maydell]

On Fri, 27 Nov 2020 at 10:34, Daniel P. Berrangé  wrote:
>
> Signed-off-by: Daniel P. Berrangé 
> ---
>  docs/system/tls.rst | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/docs/system/tls.rst b/docs/system/tls.rst
> index dc2b94257f..b0973afe1b 100644
> --- a/docs/system/tls.rst
> +++ b/docs/system/tls.rst
> @@ -64,7 +64,7 @@ interactive prompts from certtool::
> cert_signing_key
> EOF
> # certtool --generate-self-signed \
> -  --load-privkey ca-key.pem
> +  --load-privkey ca-key.pem \
>--template ca.info \
>--outfile ca-cert.pem
>

Reviewed-by: Peter Maydell 

thanks
-- PMM

Re: [PATCH 4/8] arm: Synchronize CPU on PSCI on

[Alexander Graf]

On 27.11.20 05:41, Paolo Bonzini wrote:

On 27/11/20 00:32, Alexander Graf wrote:


On 26.11.20 23:26, Peter Maydell wrote:

On Thu, 26 Nov 2020 at 22:16, Alexander Graf  wrote:
cpu_synchronize_state() sets the CPU registers into "dirty" state 
which

means that env now holds the current copy. On the next entry, we then
sync them back into HVF.

Without the cpu_synchronize_state() call, HVF never knows that the CPU
state is actually dirty. I guess it could as well live in cpu_reset()
somewhere, but we have to get the state switched over to dirty one way
or another.

One interesting thing to note here is that the CPU actually comes 
up in

"dirty" after init. But init is done on realization already. I'm not
sure why we lose the dirty state in between that and the reset.

Yeah, it sounds like you need to figure out where the dirty
to not-dirty transitions ought to be happening rather than
just fudging things here...



When init is complete (system is ready to launch), the CPU state is 
pushed to HVF and dirty is set to false. So by design, a normal 
cpu_reset doesn't have vcpu_dirty set.


How about this patch instead?

Alex



commit 8c61bc4d613b01e251b6b2f892d1a55a333c6e37
Author: Alexander Graf 
Date:   Thu Nov 26 02:47:09 2020 +0100

 hvf: arm: Mark CPU as dirty on reset

 When clearing internal state of a CPU, we should also make sure 
that HVF

 knows about it and can push the new values down to vcpu state.

 Make sure that with HVF enabled, we tell it that it should 
synchronize

 CPU state on next entry after a reset.

 This fixes PSCI handling, because now newly pushed state such as 
X0 and

 PC on remote CPU enablement also get pushed into HVF.

 Signed-off-by: Alexander Graf 

diff --git a/target/arm/arm-powerctl.c b/target/arm/arm-powerctl.c
index b75f813b40..a49a5b32e6 100644
--- a/target/arm/arm-powerctl.c
+++ b/target/arm/arm-powerctl.c
@@ -15,6 +15,7 @@
  #include "arm-powerctl.h"
  #include "qemu/log.h"
  #include "qemu/main-loop.h"
+#include "sysemu/hw_accel.h"

  #ifndef DEBUG_ARM_POWERCTL
  #define DEBUG_ARM_POWERCTL 0
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index db6f7c34ed..9a501ea4bd 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -411,6 +411,8 @@ static void arm_cpu_reset(DeviceState *dev)
  #ifndef CONFIG_USER_ONLY
  if (kvm_enabled()) {
  kvm_arm_reset_vcpu(cpu);
+    } else if (hvf_enabled()) {
+    s->vcpu_dirty = true;
  }
  #endif



Why only for HVF and only for ARM?  For example hax_init_vcpu and 
whpx_init_vcpu both set s->vcpu_dirty; should you just set it 
unconditionally in cpu_common_reset?



Mostly because there is a lot of super fragile logic all over resets 
atm. Init setts dirty, post-init clears it. Then the arch reset handlers 
assume that state is not dirty and fiddle with KVM reset ioctls and KVM 
register modification ioctls directly. Mostly because KVM for the most 
part implements its own reset logic.


I'm fairy sure I'd break someone unintentionally if I just throw this 
into the common reset handler.


However, if we're happy to fix the fallout when it arises, I'm happy to 
do so.



Alex

Re: [PATCH 28/36] vl: allow -incoming defer with -preconfig

[Igor Mammedov]

On Mon, 23 Nov 2020 09:14:27 -0500
Paolo Bonzini  wrote:

> Now that there is no RUN_STATE_PRECONFIG anymore that can conflict with
> RUN_STATE_INMIGRATE, we can allow -incoming defer with -preconfig.
> 
> Signed-off-by: Paolo Bonzini 

Reviewed-by: Igor Mammedov 

> ---
>  softmmu/vl.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index aa11fc4871..0ba1fcdb3d 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -3200,9 +3200,8 @@ static void qemu_validate_options(void)
>   "mutually exclusive");
>  exit(EXIT_FAILURE);
>  }
> -if (incoming && preconfig_requested) {
> -error_report("'preconfig' and 'incoming' options are "
> - "mutually exclusive");
> +if (incoming && preconfig_requested && strcmp(incoming, "defer") != 0) {
> +error_report("'preconfig' supports '-incoming defer' only");
>  exit(EXIT_FAILURE);
>  }
>  

Re: [PATCH for-6.0 6/6] qapi: Deprecate 'query-kvm'

[Daniel P . Berrangé]

Copying libvir-list for the deprecation warning.


On Mon, Nov 16, 2020 at 04:10:11PM +0300, Roman Bolshakov wrote:
> 'query-accel' QMP command should be used instead.
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  qapi/machine.json | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

docs/system/deprecated.rst needs to be updated for any deprecation
to be visible to consumers of QEMU.


> 
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 5648d8d24d..130b0dbebc 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -579,6 +579,9 @@
>  #
>  # Returns information about KVM acceleration
>  #
> +# Features:
> +# @deprecated: This command is deprecated, use 'query-accel' instead.
> +#
>  # Returns: @AccelInfo
>  #
>  # Since: 0.14.0
> @@ -589,7 +592,8 @@
>  # <- { "return": { "enabled": true, "present": true } }
>  #
>  ##
> -{ 'command': 'query-kvm', 'returns': 'AccelInfo' }
> +{ 'command': 'query-kvm', 'returns': 'AccelInfo',
> +  'features': [ 'deprecated' ] }
>  
>  ##
>  # @query-accel:
> -- 
> 2.29.2
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

Re: [PATCH 26/36] remove preconfig state

[Igor Mammedov]

On Fri, 27 Nov 2020 06:19:51 +0100
Paolo Bonzini  wrote:

> On 26/11/20 19:55, Igor Mammedov wrote:
> > On Mon, 23 Nov 2020 09:14:25 -0500
> > Paolo Bonzini  wrote:
> >   
> >> The preconfig state is only used if -incoming is not specified, which
> >> makes the RunState state machine more tricky than it need be.  However
> >> there is already an equivalent condition which works even with -incoming,
> >> namely qdev_hotplug.  Use it instead of a separate runstate.
> >>
> >> Signed-off-by: Paolo Bonzini 
> >> ---
> >>   hw/core/machine-qmp-cmds.c|  5 ++---
> >>   include/qapi/qmp/dispatch.h   |  1 +
> >>   monitor/hmp.c |  7 ---
> >>   monitor/qmp-cmds.c|  5 ++---
> >>   qapi/qmp-dispatch.c   |  5 +
> >>   qapi/run-state.json   |  5 +
> >>   softmmu/qdev-monitor.c| 12 
> >>   softmmu/vl.c  | 13 ++---
> >>   stubs/meson.build |  1 +
> >>   stubs/qmp-command-available.c |  7 +++
> >>   tests/qtest/qmp-test.c|  2 +-
> >>   11 files changed, 34 insertions(+), 29 deletions(-)
> >>   create mode 100644 stubs/qmp-command-available.c
> >>
> >> diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c
> >> index 5362c80a18..cb9387c5f5 100644
> >> --- a/hw/core/machine-qmp-cmds.c
> >> +++ b/hw/core/machine-qmp-cmds.c
> >> @@ -286,9 +286,8 @@ HotpluggableCPUList *qmp_query_hotpluggable_cpus(Error 
> >> **errp)
> >>   
> >>   void qmp_set_numa_node(NumaOptions *cmd, Error **errp)
> >>   {
> >> -if (!runstate_check(RUN_STATE_PRECONFIG)) {
> >> -error_setg(errp, "The command is permitted only in '%s' state",
> >> -   RunState_str(RUN_STATE_PRECONFIG));
> >> +if (qdev_hotplug) {  
> > 
> > that would work only as long as qemu_init_board() hasn't been called,
> > and fall apart as soon as we permit creating cold-pluged devices
> > (qemu_create_cli_devices()) at preconfig stage.
> > 
> > for qmp_set_numa_node() the better fit would something like
> >if(is_board_created)
> >   error_out
> > so it won't break silently when we start extending list of
> > commands allowed at preconfig time.
> >   
> >> + error_setg(errp, "The command is permitted only before the 
> >> machine has been created");
> >>return;
> >>   }  
> 
> I don't understand...  qdev_hotplug is a bad name for is_board_created, 
> it is set by qdev_machine_creation_done which is called after preconfig 
> is left.  As of this patch that happens after the early 
> qemu_main_loop(); the next patch moves it to qmp_x_exit_preconfig.
it works in context of this series since

  +qemu_init_board();
  +qemu_create_cli_devices();
  +qemu_machine_creation_done();

are called within the same command qmp_x_exit_preconfig, 
if preconfig is enabled we happen to call qmp_set_numa_node()
and if (qdev_hotplug) {error} work as expected, since qemu_init_board()
hasn't been called yet.

but I'm thinking about what happens beyond this series, when we start
splitting qmp_x_exit_preconfig() after this series on separate stages.

By using qdev_hotplug here, we practically loose dependency tracking
on qemu_init_board() not being yet called. And if later we forget that,
then it would allow to call qmp_set_numa_node() after qemu_init_board()
but before qemu_machine_creation_done()

So for this intermediate stage, instead of abusing qdev_hotplug adding
a temporary is_board_created might be used. And when we introduce
new phases you've described below, is_board_created could be replaced
with appropriate phase check.


> Cold-plugged devices would (by definition) be created while qdev_hotplug 
> is false.  But before we get there, I will have replaced the two states 
> permitted by qdev_hotplug with five separate phases (PHASE_NO_MACHINE, 
> PHASE_MACHINE_CREATED, PHASE_ACCEL_CREATED, PHASE_MACHINE_INITIALIZED, 
> PHASE_MACHINE_READY) to clarify the QMP command implementation and to 
> assert that various functions are called in the right phase.
> 
> Thanks,
> 
> Paolo
> 

Re: [PATCH for-6.0 2/6] qapi: Rename KvmInfo to AccelInfo

[Dr. David Alan Gilbert]

* Roman Bolshakov (r.bolsha...@yadro.com) wrote:
> There's nothing specific to KVM in the structure. A more generic name
> would be more appropriate.
> 
> Signed-off-by: Roman Bolshakov 

For HMP:

Acked-by: Dr. David Alan Gilbert 

Markus/Eric: Is it OK from QMP to change the type like that?

Dave

> ---
>  monitor/hmp-cmds.c |  4 ++--
>  monitor/qmp-cmds.c |  8 
>  qapi/machine.json  | 18 +-
>  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index a6a6684df1..ea86289fe8 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -120,7 +120,7 @@ void hmp_info_version(Monitor *mon, const QDict *qdict)
>  
>  void hmp_info_kvm(Monitor *mon, const QDict *qdict)
>  {
> -KvmInfo *info;
> +AccelInfo *info;
>  
>  info = qmp_query_kvm(NULL);
>  monitor_printf(mon, "kvm support: ");
> @@ -130,7 +130,7 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict)
>  monitor_printf(mon, "not compiled\n");
>  }
>  
> -qapi_free_KvmInfo(info);
> +qapi_free_AccelInfo(info);
>  }
>  
>  void hmp_info_status(Monitor *mon, const QDict *qdict)
> diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> index 0454394e76..f5d50afa9c 100644
> --- a/monitor/qmp-cmds.c
> +++ b/monitor/qmp-cmds.c
> @@ -52,9 +52,9 @@ NameInfo *qmp_query_name(Error **errp)
>  return info;
>  }
>  
> -KvmInfo *qmp_query_kvm(Error **errp)
> +AccelInfo *qmp_query_kvm(Error **errp)
>  {
> -KvmInfo *info = g_malloc0(sizeof(*info));
> +AccelInfo *info = g_malloc0(sizeof(*info));
>  
>  info->enabled = kvm_enabled();
>  info->present = kvm_available();
> @@ -62,9 +62,9 @@ KvmInfo *qmp_query_kvm(Error **errp)
>  return info;
>  }
>  
> -KvmInfo *qmp_query_accel(const char *name, Error **errp)
> +AccelInfo *qmp_query_accel(const char *name, Error **errp)
>  {
> -KvmInfo *info = g_malloc0(sizeof(*info));
> +AccelInfo *info = g_malloc0(sizeof(*info));
>  
>  AccelClass *ac = accel_find(name);
>  
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 11f364fab4..5648d8d24d 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -562,24 +562,24 @@
>  { 'command': 'inject-nmi' }
>  
>  ##
> -# @KvmInfo:
> +# @AccelInfo:
>  #
> -# Information about support for KVM acceleration
> +# Information about support for an acceleration
>  #
> -# @enabled: true if KVM acceleration is active
> +# @enabled: true if an acceleration is active
>  #
> -# @present: true if KVM acceleration is built into this executable
> +# @present: true if an acceleration is built into this executable
>  #
>  # Since: 0.14.0
>  ##
> -{ 'struct': 'KvmInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
> +{ 'struct': 'AccelInfo', 'data': {'enabled': 'bool', 'present': 'bool'} }
>  
>  ##
>  # @query-kvm:
>  #
>  # Returns information about KVM acceleration
>  #
> -# Returns: @KvmInfo
> +# Returns: @AccelInfo
>  #
>  # Since: 0.14.0
>  #
> @@ -589,14 +589,14 @@
>  # <- { "return": { "enabled": true, "present": true } }
>  #
>  ##
> -{ 'command': 'query-kvm', 'returns': 'KvmInfo' }
> +{ 'command': 'query-kvm', 'returns': 'AccelInfo' }
>  
>  ##
>  # @query-accel:
>  #
>  # Returns information about an accelerator
>  #
> -# Returns: @KvmInfo
> +# Returns: @AccelInfo
>  #
>  # Since: 6.0.0
>  #
> @@ -608,7 +608,7 @@
>  ##
>  { 'command': 'query-accel',
>'data': { 'name': 'str' },
> -  'returns': 'KvmInfo' }
> +  'returns': 'AccelInfo' }
>  
>  ##
>  # @NumaOptionsType:
> -- 
> 2.29.2
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH for-6.0 5/6] hmp: Add 'info accel' command

[Dr. David Alan Gilbert]

* Roman Bolshakov (r.bolsha...@yadro.com) wrote:
> The command is made after 'info kvm' and aims to replace it as more
> generic one.
> 
> If used without parameters, the command can used to get current
> accelerator. Otherwise, it may be used to determine if an accelerator is
> available. Here's an example if a VM with hvf accel is started:
> 
>   (qemu) info accel
>   hvf support: enabled
>   (qemu) info accel kvm
>   kvm support: not compiled
>   (qemu) info accel tcg
>   tcg support: disabled
> 
> Signed-off-by: Roman Bolshakov 
> ---
>  hmp-commands-info.hx  | 13 +
>  include/monitor/hmp.h |  1 +
>  monitor/hmp-cmds.c| 32 
>  3 files changed, 46 insertions(+)
> 
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index 117ba25f91..e9da6b52e4 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -337,6 +337,19 @@ SRST
>  Show KVM information.
>  ERST
>  
> +{
> +.name   = "accel",
> +.args_type  = "name:s?",
> +.params = "[name]",
> +.help   = "show accelerator information",
> +.cmd= hmp_info_accel,
> +},
> +
> +SRST
> +  ``info accel``` [*name*]
> +Show accelerator information.
> +ERST
> +
>  {
>  .name   = "numa",
>  .args_type  = "",
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index ed2913fd18..03f22957d9 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -21,6 +21,7 @@ void hmp_handle_error(Monitor *mon, Error *err);
>  void hmp_info_name(Monitor *mon, const QDict *qdict);
>  void hmp_info_version(Monitor *mon, const QDict *qdict);
>  void hmp_info_kvm(Monitor *mon, const QDict *qdict);
> +void hmp_info_accel(Monitor *mon, const QDict *qdict);
>  void hmp_info_status(Monitor *mon, const QDict *qdict);
>  void hmp_info_uuid(Monitor *mon, const QDict *qdict);
>  void hmp_info_chardev(Monitor *mon, const QDict *qdict);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index ea86289fe8..00db791aa3 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -20,6 +20,7 @@
>  #include "chardev/char.h"
>  #include "sysemu/block-backend.h"
>  #include "sysemu/runstate.h"
> +#include "sysemu/accel.h"
>  #include "qemu/config-file.h"
>  #include "qemu/option.h"
>  #include "qemu/timer.h"
> @@ -133,6 +134,37 @@ void hmp_info_kvm(Monitor *mon, const QDict *qdict)
>  qapi_free_AccelInfo(info);
>  }
>  
> +void hmp_info_accel(Monitor *mon, const QDict *qdict)
> +{
> +AccelInfo *info;
> +AccelClass *acc;
> +const char *name, *typename;
> +char *current_name;
> +int len;
> +
> +/* Figure out current accelerator */
> +acc = ACCEL_GET_CLASS(current_accel());

Is that always guaranteed non-null?

> +typename = object_class_get_name(OBJECT_CLASS(acc));
> +len = strlen(typename) - strlen(ACCEL_CLASS_SUFFIX);
> +current_name = g_strndup(typename, len);
> +
> +name = qdict_get_try_str(qdict, "name");
> +if (!name) {
> +name = current_name;
> +}
> +
> +info = qmp_query_accel(name, NULL);
> +monitor_printf(mon, "%s support: ", name);
> +if (info->present) {
> +monitor_printf(mon, "%s\n", info->enabled ? "enabled" : "disabled");
> +} else {
> +monitor_printf(mon, "not compiled\n");
> +}
> +
> +qapi_free_AccelInfo(info);
> +g_free(current_name);
> +}

I think that's fine, since HMP is not guaranteed stable, I'd say it's
fine to kill off 'info kvm' and replace it with this.


Reviewed-by: Dr. David Alan Gilbert 


Dave

> +
>  void hmp_info_status(Monitor *mon, const QDict *qdict)
>  {
>  StatusInfo *info;
> -- 
> 2.29.2
> 
-- 
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[qemu PATCH] docs: fix missing backslash in certtool shell example

[Daniel P . Berrangé]

Signed-off-by: Daniel P. Berrangé 
---
 docs/system/tls.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/system/tls.rst b/docs/system/tls.rst
index dc2b94257f..b0973afe1b 100644
--- a/docs/system/tls.rst
+++ b/docs/system/tls.rst
@@ -64,7 +64,7 @@ interactive prompts from certtool::
cert_signing_key
EOF
# certtool --generate-self-signed \
-  --load-privkey ca-key.pem
+  --load-privkey ca-key.pem \
   --template ca.info \
   --outfile ca-cert.pem
 
-- 
2.28.0

Re: ImageInfo oddities regarding compression

[Kevin Wolf]

Am 27.11.2020 um 11:14 hat Daniel P. Berrangé geschrieben:
> On Fri, Nov 27, 2020 at 11:06:36AM +0100, Markus Armbruster wrote:
> > ImageInfo has an optional member @compressed:
> > 
> > ##
> > # @ImageInfo:
> > [...]
> > # @compressed: true if the image is compressed (Since 1.7)
> > 
> > Doc bug: neglects to specify the default.  I guess it's false.

I think rather than false (definitely not compressed) it might just mean
unknown, see below.

> > The only user appears to be vmdk_get_extent_info().  Goes back to
> > v1.7.0's commits
> > 
> > f4c129a38a vmdk: Implment bdrv_get_specific_info
> > cbe82d7fb3 qapi: Add optional field 'compressed' to ImageInfo
> > 
> > ImageInfo also has an optional member @format-specific.
> > 
> > Doc bug: neglects to specify when it's present.  I assume it's always
> > present when member @format has a value that has a non-empty variant in
> > @format-specific's type ImageInfoSpecific.

This seems to be the intention, yes.

> > Format qcow2's variant is ImageInfoSpecificQCow2.  It has a mandatory
> > member @compression-type.
> > 
> >##
> ># @Qcow2CompressionType:
> >#
> ># Compression type used in qcow2 image file
> >#
> ># @zlib: zlib compression, see 
> ># @zstd: zstd compression, see 
> >#
> ># Since: 5.1
> >##
> >{ 'enum': 'Qcow2CompressionType',
> >  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
> > 
> > Apparently, you can't have a qcow2 image without compression.  Correct?
> > 
> > Can you imagine a use case for "without compression"?
> 
> Yes & no. An image always has a compression type, either implicitly
> zlib as the historical default, or explicitly as a type recorded in
> the header.  This doesn't mean compression is used.
> 
> There may be zero or more clusters actually using compression.
> Typically compression will never be used, but there's still an
> associated compression type regardless.

Right, so the correct answer to "is this image compressed?" is "unknown"
for qcow2. Providing an answer would require checking all clusters in
the image for the compression flag, which is not something we want to do
for query-block. And even if we did that, it would still be unclear what
to do with a partially compressed image.

The @compression-type only tells you what format a compressed cluster
uses if any compressed cluster exists in the image (or if a new
compressed cluster is written to it). It doesn't mean that such clusters
currently exist.

Kevin

Re: [PATCH 13/36] vl: move semihosting command line fallback to qemu_init_board

[Igor Mammedov]

On Fri, 27 Nov 2020 06:03:43 +0100
Paolo Bonzini  wrote:

> On 26/11/20 18:10, Igor Mammedov wrote:
> > On Mon, 23 Nov 2020 09:14:12 -0500
> > Paolo Bonzini  wrote:
> >   
> >> Move more sane parts of the huge qemu_init function out of it.
> >>
> >> Signed-off-by: Paolo Bonzini 
> >> ---
> >>   softmmu/vl.c | 12 +---
> >>   1 file changed, 5 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/softmmu/vl.c b/softmmu/vl.c
> >> index ab08a0290c..5d68cf828c 100644
> >> --- a/softmmu/vl.c
> >> +++ b/softmmu/vl.c
> >> @@ -3070,6 +3070,11 @@ static void qemu_init_board(void)
> >>   {
> >>   MachineClass *machine_class = MACHINE_GET_CLASS(current_machine);
> >>   
> >> +if (semihosting_enabled() && !semihosting_get_argc() && 
> >> current_machine->kernel_filename) {
> >> +/* fall back to the -kernel/-append */
> >> +semihosting_arg_fallback(current_machine->kernel_filename, 
> >> current_machine->kernel_cmdline);
> >> +}  
> > 
> > it doesn't seem to depend on anything that warrants calling it this late.  
> 
> Yes, calling it around machine initialization time is also a 
> possibility.  I just wanted to get rid of it in code that I'm actually 
> looking at. :)

I'd prefer it being moved close to CLI parsing,
in a place where other _early call go.

We probably want qemu_init_board() being clear of not really needed clutter.

> 
> Paolo
> 
> >   
> >>   if (machine_class->default_ram_id && current_machine->ram_size &&
> >>   numa_uses_legacy_mem() && !current_machine->ram_memdev_id) {
> >>   create_default_memdev(current_machine, mem_path);
> >> @@ -4385,13 +4390,6 @@ void qemu_init(int argc, char **argv, char **envp)
> >>   boot_order = machine_class->default_boot_order;
> >>   }
> >>   
> >> -if (semihosting_enabled() && !semihosting_get_argc()) {
> >> -const char *kernel_filename = qemu_opt_get(machine_opts, 
> >> "kernel");
> >> -const char *kernel_cmdline = qemu_opt_get(machine_opts, "append");
> >> -/* fall back to the -kernel/-append */
> >> -semihosting_arg_fallback(kernel_filename, kernel_cmdline);
> >> -}  
> > 
> > Can we move this hunk as is to somewhere around qemu_maybe_daemonize() time?
> > 
> >   
> >>   if (net_init_clients() < 0) {
> >>   error_report_err(err);
> >>   exit(1);  
> >   
> 

Re: ImageInfo oddities regarding compression

[Daniel P . Berrangé]

On Fri, Nov 27, 2020 at 11:06:36AM +0100, Markus Armbruster wrote:
> ImageInfo has an optional member @compressed:
> 
> ##
> # @ImageInfo:
> [...]
> # @compressed: true if the image is compressed (Since 1.7)
> 
> Doc bug: neglects to specify the default.  I guess it's false.
> 
> The only user appears to be vmdk_get_extent_info().  Goes back to
> v1.7.0's commits
> 
> f4c129a38a vmdk: Implment bdrv_get_specific_info
> cbe82d7fb3 qapi: Add optional field 'compressed' to ImageInfo
> 
> ImageInfo also has an optional member @format-specific.
> 
> Doc bug: neglects to specify when it's present.  I assume it's always
> present when member @format has a value that has a non-empty variant in
> @format-specific's type ImageInfoSpecific.
> 
> Format qcow2's variant is ImageInfoSpecificQCow2.  It has a mandatory
> member @compression-type.
> 
>##
># @Qcow2CompressionType:
>#
># Compression type used in qcow2 image file
>#
># @zlib: zlib compression, see 
># @zstd: zstd compression, see 
>#
># Since: 5.1
>##
>{ 'enum': 'Qcow2CompressionType',
>  'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
> 
> Apparently, you can't have a qcow2 image without compression.  Correct?
> 
> Can you imagine a use case for "without compression"?

Yes & no. An image always has a compression type, either implicitly
zlib as the historical default, or explicitly as a type recorded in
the header.  This doesn't mean compression is used.

There may be zero or more clusters actually using compression.
Typically compression will never be used, but there's still an
associated compression type regardless.


Regards,
Daniel
-- 
|: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

ImageInfo oddities regarding compression

[Markus Armbruster]

ImageInfo has an optional member @compressed:

##
# @ImageInfo:
[...]
# @compressed: true if the image is compressed (Since 1.7)

Doc bug: neglects to specify the default.  I guess it's false.

The only user appears to be vmdk_get_extent_info().  Goes back to
v1.7.0's commits

f4c129a38a vmdk: Implment bdrv_get_specific_info
cbe82d7fb3 qapi: Add optional field 'compressed' to ImageInfo

ImageInfo also has an optional member @format-specific.

Doc bug: neglects to specify when it's present.  I assume it's always
present when member @format has a value that has a non-empty variant in
@format-specific's type ImageInfoSpecific.

Format qcow2's variant is ImageInfoSpecificQCow2.  It has a mandatory
member @compression-type.

   ##
   # @Qcow2CompressionType:
   #
   # Compression type used in qcow2 image file
   #
   # @zlib: zlib compression, see 
   # @zstd: zstd compression, see 
   #
   # Since: 5.1
   ##
   { 'enum': 'Qcow2CompressionType',
 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }

Apparently, you can't have a qcow2 image without compression.  Correct?

Can you imagine a use case for "without compression"?

I fell down this (thankfully shallow) rabbit hole because we also have

{ 'enum': 'MultiFDCompression',
  'data': [ 'none', 'zlib',
{ 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }

I wonder whether we could merge them into a common type.

Re: [PATCH v4 0/6] UFFD write-tracking migration/snapshots

[Andrey Gruzdev]

On 27.11.2020 12:49, Peter Krempa wrote:

On Fri, Nov 27, 2020 at 11:21:39 +0300, Andrey Gruzdev wrote:

On 26.11.2020 18:47, Peter Krempa wrote:

On Thu, Nov 26, 2020 at 18:17:28 +0300, Andrey Gruzdev via wrote:

This patch series is a kind of 'rethinking' of Denis Plotnikov's ideas he's
implemented in his series '[PATCH v0 0/4] migration: add background snapshot'.

Currently the only way to make (external) live VM snapshot is using existing
dirty page logging migration mechanism. The main problem is that it tends to
produce a lot of page duplicates while running VM goes on updating already
saved pages. That leads to the fact that vmstate image size is commonly several
times bigger then non-zero part of virtual machine's RSS. Time required to
converge RAM migration and the size of snapshot image severely depend on the
guest memory write rate, sometimes resulting in unacceptably long snapshot
creation time and huge image size.

This series propose a way to solve the aforementioned problems. This is done
by using different RAM migration mechanism based on UFFD write protection
management introduced in v5.7 kernel. The migration strategy is to 'freeze'
guest RAM content using write-protection and iteratively release protection
for memory ranges that have already been saved to the migration stream.
At the same time we read in pending UFFD write fault events and save those
pages out-of-order with higher priority.


This sounds amazing! Based on your description I assume that the final
memory image contains state image from the beginning of the migration.

This would make it desirable for the 'migrate' qmp command to be used as
part of a 'transaction' qmp command so that we can do an instant disk
and memory snapshot without any extraneous pausing of the VM.

I'll have a look at using this mechanism in libvirt natively.



Correct, the final image contains state at the beginning of migration.

So far, if I'm not missing something about libvirt, for external snapshot
creation it performs a sequence like that:
migrate(fd)->transaction(blockdev-snapshot-all)->cont.

So, in case 'background-snapshot' capability is enabled, the sequence would
change to:
stop->transaction(blockdev-snapshot-all)->migrate(fd).
With background snapshot migration it will finish with VM running so there's
not need to 'cont' here.


Yes, that's correct.

The reason I've suggested that 'migrate' being part of a 'transaction'
is that it would remove the need to stop it for the disk snapshot part.



Hmm, I believe stopping VM for a short time is unavoidable to keep saved 
device state consistent with blockdev snapshot base.. May be I've missed 
something but it seems logical.


--
Andrey Gruzdev, Principal Engineer
Virtuozzo GmbH  +7-903-247-6397
virtuzzo.com

Re: [PATCH v4 0/6] UFFD write-tracking migration/snapshots

[Peter Krempa]

On Fri, Nov 27, 2020 at 11:21:39 +0300, Andrey Gruzdev wrote:
> On 26.11.2020 18:47, Peter Krempa wrote:
> > On Thu, Nov 26, 2020 at 18:17:28 +0300, Andrey Gruzdev via wrote:
> > > This patch series is a kind of 'rethinking' of Denis Plotnikov's ideas 
> > > he's
> > > implemented in his series '[PATCH v0 0/4] migration: add background 
> > > snapshot'.
> > > 
> > > Currently the only way to make (external) live VM snapshot is using 
> > > existing
> > > dirty page logging migration mechanism. The main problem is that it tends 
> > > to
> > > produce a lot of page duplicates while running VM goes on updating already
> > > saved pages. That leads to the fact that vmstate image size is commonly 
> > > several
> > > times bigger then non-zero part of virtual machine's RSS. Time required to
> > > converge RAM migration and the size of snapshot image severely depend on 
> > > the
> > > guest memory write rate, sometimes resulting in unacceptably long snapshot
> > > creation time and huge image size.
> > > 
> > > This series propose a way to solve the aforementioned problems. This is 
> > > done
> > > by using different RAM migration mechanism based on UFFD write protection
> > > management introduced in v5.7 kernel. The migration strategy is to 
> > > 'freeze'
> > > guest RAM content using write-protection and iteratively release 
> > > protection
> > > for memory ranges that have already been saved to the migration stream.
> > > At the same time we read in pending UFFD write fault events and save those
> > > pages out-of-order with higher priority.
> > 
> > This sounds amazing! Based on your description I assume that the final
> > memory image contains state image from the beginning of the migration.
> > 
> > This would make it desirable for the 'migrate' qmp command to be used as
> > part of a 'transaction' qmp command so that we can do an instant disk
> > and memory snapshot without any extraneous pausing of the VM.
> > 
> > I'll have a look at using this mechanism in libvirt natively.
> > 
> 
> Correct, the final image contains state at the beginning of migration.
> 
> So far, if I'm not missing something about libvirt, for external snapshot
> creation it performs a sequence like that:
> migrate(fd)->transaction(blockdev-snapshot-all)->cont.
> 
> So, in case 'background-snapshot' capability is enabled, the sequence would
> change to:
> stop->transaction(blockdev-snapshot-all)->migrate(fd).
> With background snapshot migration it will finish with VM running so there's
> not need to 'cont' here.

Yes, that's correct.

The reason I've suggested that 'migrate' being part of a 'transaction'
is that it would remove the need to stop it for the disk snapshot part. 

Re: [PATCH 1/6] migration: Add multi-thread compress method

[Markus Armbruster]

Kevin, Max, suggest to skip right to Qcow2CompressionType.

Zeyu Jin  writes:

> A multi-thread compress method parameter is added to hold the method we
> are going to use. By default the 'zlib' method is used to maintain the
> compatibility as before.
>
> Signed-off-by: Zeyu Jin 
> Signed-off-by: Ying Fang 
[...]
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 3c75820527..2ed6a55b92 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -525,6 +525,19 @@
>'data': [ 'none', 'zlib',
>  { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>  
> +##
> +# @CompressMethod:
> +#
> +# An enumeration of multi-thread compression methods.
> +#
> +# @zlib: use zlib compression method.
> +#
> +# Since: 6.0
> +#
> +##
> +{ 'enum': 'CompressMethod',
> +  'data': [ 'zlib' ] }
> +
>  ##
>  # @BitmapMigrationBitmapAlias:
>  #
> @@ -599,6 +612,9 @@
>  #  compression, so set the decompress-threads to the 
> number about 1/4
>  #  of compress-threads is adequate.
>  #
> +# @compress-method: Set compression method to use in multi-thread 
> compression.
> +#   Defaults to zlib. (Since 6.0)

We already have @multifd-compression.  Why do we need to control the two
separately?  Can you give a use case for different settings?

If we do want two parameters: the names @compress-method and
@multifd-compression are inconsistent.  According to your comment,
@compress-method applies only to multi-thread compression.  That leads
me to suggest renaming it to @multi-thread-compression.

After PATCH 4, CompressMethod is almost the same as MultiFDCompression:

   { 'enum': 'CompressMethod',
 'data': [ 'zlib' ] }
 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }

   { 'enum': 'MultiFDCompression',
 'data': [ 'none', 'zlib',
   { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }

The difference is member 'none'.  Why does compression 'none' make sense
for multi-fd, but not for multi-thread?

If the difference is wanted: the names are inconsistent.  Less of an
issue than member names, because type names are not externally visible.
Let's make them consistent anyway.

Hmm, there's also Qcow2CompressionType.  That's another conversation;
I'll start a new thread for it.

[...]

Re: [PATCH v2] qga/commands-posix: Send CCW address on s390x with the fsinfo data

[Cornelia Huck]

On Fri, 27 Nov 2020 09:23:53 +0100
Thomas Huth  wrote:

> We need the CCW address on the libvirt side to correctly identify
> the disk, so add this information to the GuestDiskAddress on s390x.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1755075
> Signed-off-by: Thomas Huth 
> ---
>  v2: Add missing comment about "subchno" (thanks to Cornelia for spotting 
> this)
> 
>  The libirt part of this fix can be found here:
>  https://www.redhat.com/archives/libvir-list/2020-November/msg01455.html
> 
>  qga/commands-posix.c | 34 ++
>  qga/qapi-schema.json | 20 +++-
>  2 files changed, 53 insertions(+), 1 deletion(-)

Reviewed-by: Cornelia Huck 

[PATCH v2 0/6] migration: Multi-thread compression method support

[Zeyu Jin]

Currently we have both multi-thread compression and multifd to optimize
live migration in Qemu. Mulit-thread compression deals with the situation
where network bandwith is limited but cpu resource adequate. Multifd instead
aims to take full advantage of network bandwith. Moreover it supports both
zlib and zstd compression on each channel.

In this patch series, we did some code refactoring on multi-thread compression
live migration and bring zstd compression method support for it.

Below is the test result of multi-thread compression live migration
with different compress methods. Test result shows that zstd outperforms
zlib by about 70%.

 Migration Configuration:
 Guest 8U 32G
 compress-threads   8
 decompress-threads 2
 compress-level 1
 bandwidth-limit 100Mbps

 Test Result:
 +-+--+-+
 |  compress method|   zlib   |zstd |
 +-+--+-+
 |  total time (ms)|   75256  |44187|
 +-+--+-+
 |  downtime(ms)   |   128|81   |
 +-+--+-+
 |  transferred ram(kB)|   1576866|736117   |
 +-+--+-+
 |  throughput(mbps)   |   172.06 |137.16   |
 +-+--+-+
 |  total ram(kB)  |   33685952   |33685952 |
 +-+--+-+

Zeyu Jin (6):
  migration: Add multi-thread compress method
  migration: Refactoring multi-thread compress migration
  migration: Add multi-thread compress ops
  migration: Add zstd support in multi-thread compression
  migration: Add compress_level sanity check
  doc: Update multi-thread compression doc

 docs/multi-thread-compression.txt |  31 ++-
 hw/core/qdev-properties-system.c  |  11 +
 include/hw/qdev-properties.h  |   4 +
 migration/migration.c |  56 -
 migration/migration.h |   1 +
 migration/qemu-file.c |  62 +
 migration/qemu-file.h |   4 +-
 migration/ram.c   | 381 +-
 monitor/hmp-cmds.c|  12 +
 qapi/migration.json   |  26 +-
 10 files changed, 465 insertions(+), 123 deletions(-)

-- 
2.27.0

[PATCH v2 5/6] migration: Add compress_level sanity check

[Zeyu Jin]

Zlib compression has level from 1 to 9. However Zstd compression has level
from 1 to 22 (level >= 20 not recommanded). Let's do sanity check here
to make sure a vaild compress_level is given by user.

Signed-off-by: Zeyu Jin 
Signed-off-by: Ying Fang 
---
 migration/migration.c | 32 
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index c11f137a8d..fab26084c2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1235,16 +1235,40 @@ void 
qmp_migrate_set_capabilities(MigrationCapabilityStatusList *params,
 }
 }
 
+static bool compress_level_check(MigrationParameters *params, Error **errp)
+{
+switch (params->compress_method) {
+case COMPRESS_METHOD_ZLIB:
+if (params->compress_level > 9 || params->compress_level < 1) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
+   "a value in the range of 0 to 9 for Zlib method");
+return false;
+}
+break;
+#ifdef CONFIG_ZSTD
+case COMPRESS_METHOD_ZSTD:
+if (params->compress_level > 19 || params->compress_level < 1) {
+error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
+"a value in the range of 1 to 19 for Zstd method");
+return false;
+}
+break;
+#endif
+default:
+error_setg(errp, "Checking compress_level failed for unknown reason");
+return false;
+}
+
+return true;
+}
+
 /*
  * Check whether the parameters are valid. Error will be put into errp
  * (if provided). Return true if valid, otherwise false.
  */
 static bool migrate_params_check(MigrationParameters *params, Error **errp)
 {
-if (params->has_compress_level &&
-(params->compress_level > 9)) {
-error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "compress_level",
-   "is invalid, it should be in the range of 0 to 9");
+if (params->has_compress_level && !compress_level_check(params, errp)) {
 return false;
 }
 
-- 
2.27.0

[PATCH v2 4/6] migration: Add zstd support in multi-thread compression

[Zeyu Jin]

This patch enables zstd option in multi-thread compression.

Signed-off-by: Zeyu Jin 
Signed-off-by: Ying Fang 
---
 hw/core/qdev-properties-system.c |   2 +-
 migration/ram.c  | 128 ++-
 qapi/migration.json  |   2 +-
 3 files changed, 129 insertions(+), 3 deletions(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index a582721a7b..b369187bdc 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -667,7 +667,7 @@ const PropertyInfo qdev_prop_multifd_compression = {
 const PropertyInfo qdev_prop_compress_method = {
 .name = "CompressMethod",
 .description = "multi-thread compression method, "
-   "zlib",
+   "zlib/zstd",
 .enum_table = _lookup,
 .get = qdev_propinfo_get_enum,
 .set = qdev_propinfo_set_enum,
diff --git a/migration/ram.c b/migration/ram.c
index 6f7fab7d4f..2550225d9a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -57,6 +57,10 @@
 #include "qemu/iov.h"
 #include "multifd.h"
 
+#ifdef CONFIG_ZSTD
+#include 
+#include 
+#endif
 /***/
 /* ram save/restore */
 
@@ -424,6 +428,11 @@ struct CompressParam {
 /* for zlib compression */
 z_stream stream;
 
+#ifdef CONFIG_ZSTD
+ZSTD_CStream *zstd_cs;
+ZSTD_inBuffer in;
+ZSTD_outBuffer out;
+#endif
 };
 typedef struct CompressParam CompressParam;
 
@@ -438,6 +447,12 @@ struct DecompressParam {
 
 /* for zlib compression */
 z_stream stream;
+
+#ifdef CONFIG_ZSTD
+ZSTD_DStream *zstd_ds;
+ZSTD_inBuffer in;
+ZSTD_outBuffer out;
+#endif
 };
 typedef struct DecompressParam DecompressParam;
 
@@ -571,6 +586,102 @@ static int zlib_check_len(int len)
 return len < 0 || len > compressBound(TARGET_PAGE_SIZE);
 }
 
+#ifdef CONFIG_ZSTD
+static int zstd_save_setup(CompressParam *param)
+{
+int res;
+param->zstd_cs = ZSTD_createCStream();
+if (!param->zstd_cs) {
+return -1;
+}
+res = ZSTD_initCStream(param->zstd_cs, migrate_compress_level());
+if (ZSTD_isError(res)) {
+return -1;
+}
+return 0;
+}
+static void zstd_save_cleanup(CompressParam *param)
+{
+ZSTD_freeCStream(param->zstd_cs);
+param->zstd_cs = NULL;
+}
+static ssize_t zstd_compress_data(CompressParam *param, size_t size)
+{
+int ret;
+uint8_t *dest = NULL;
+uint8_t *p = param->originbuf;
+QEMUFile *f = f = param->file;
+ssize_t blen = qemu_put_compress_start(f, );
+if (blen < ZSTD_compressBound(size)) {
+return -1;
+}
+param->out.dst = dest;
+param->out.size = blen;
+param->out.pos = 0;
+param->in.src = p;
+param->in.size = size;
+param->in.pos = 0;
+do {
+ret = ZSTD_compressStream2(param->zstd_cs, >out,
+   >in, ZSTD_e_end);
+} while (ret > 0 && (param->in.size - param->in.pos > 0)
+&& (param->out.size - param->out.pos > 0));
+if (ret > 0 && (param->in.size - param->in.pos > 0)) {
+return -1;
+}
+if (ZSTD_isError(ret)) {
+return -1;
+}
+blen = param->out.pos;
+qemu_put_compress_end(f, blen);
+return blen + sizeof(int32_t);
+}
+static int zstd_load_setup(DecompressParam *param)
+{
+int ret;
+param->zstd_ds = ZSTD_createDStream();
+if (!param->zstd_ds) {
+return -1;
+}
+ret = ZSTD_initDStream(param->zstd_ds);
+if (ZSTD_isError(ret)) {
+return -1;
+}
+return 0;
+}
+static void zstd_load_cleanup(DecompressParam *param)
+{
+ZSTD_freeDStream(param->zstd_ds);
+param->zstd_ds = NULL;
+}
+static int
+zstd_decompress_data(DecompressParam *param, uint8_t *dest, size_t size)
+{
+int ret;
+param->out.dst = dest;
+param->out.size = size;
+param->out.pos = 0;
+param->in.src = param->compbuf;
+param->in.size = param->len;
+param->in.pos = 0;
+do {
+ret = ZSTD_decompressStream(param->zstd_ds, >out, >in);
+} while (ret > 0 && (param->in.size - param->in.pos > 0)
+&& (param->out.size - param->out.pos > 0));
+if (ret > 0 && (param->in.size - param->in.pos > 0)) {
+return -1;
+}
+if (ZSTD_isError(ret)) {
+return -1;
+}
+return ret;
+}
+static int zstd_check_len(int len)
+{
+return len < 0 || len > ZSTD_compressBound(TARGET_PAGE_SIZE);
+}
+#endif
+
 static int set_compress_ops(void)
 {
compress_ops = g_new0(MigrationCompressOps, 1);
@@ -581,9 +692,16 @@ static int set_compress_ops(void)
 compress_ops->save_cleanup = zlib_save_cleanup;
 compress_ops->compress_data = zlib_compress_data;
 break;
+#ifdef CONFIG_ZSTD
+case COMPRESS_METHOD_ZSTD:
+compress_ops->save_setup = zstd_save_setup;
+compress_ops->save_cleanup = zstd_save_cleanup;
+compress_ops->compress_data = zstd_compress_data;
+break;
+#endif
 default:
   

[PATCH v2 3/6] migration: Add multi-thread compress ops

[Zeyu Jin]

Add the MigrationCompressOps and MigrationDecompressOps structures to make
the compression method configurable for multi-thread compression migration.

Signed-off-by: Zeyu Jin 
Signed-off-by: Ying Fang parameters.decompress_threads;
 }
 
+CompressMethod migrate_compress_method(void)
+{
+MigrationState *s;
+
+s = migrate_get_current();
+
+return s->parameters.compress_method;
+}
+
 bool migrate_dirty_bitmaps(void)
 {
 MigrationState *s;
diff --git a/migration/migration.h b/migration/migration.h
index d096b77f74..e22b2ef840 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -339,6 +339,7 @@ int migrate_compress_level(void);
 int migrate_compress_threads(void);
 int migrate_compress_wait_thread(void);
 int migrate_decompress_threads(void);
+CompressMethod migrate_compress_method(void);
 bool migrate_use_events(void);
 bool migrate_postcopy_blocktime(void);
 
diff --git a/migration/ram.c b/migration/ram.c
index 1818a56314..6f7fab7d4f 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -419,8 +419,11 @@ struct CompressParam {
 ram_addr_t offset;
 
 /* internally used fields */
-z_stream stream;
 uint8_t *originbuf;
+
+/* for zlib compression */
+z_stream stream;
+
 };
 typedef struct CompressParam CompressParam;
 
@@ -432,12 +435,29 @@ struct DecompressParam {
 void *des;
 uint8_t *compbuf;
 int len;
+
+/* for zlib compression */
 z_stream stream;
 };
 typedef struct DecompressParam DecompressParam;
 
+typedef struct {
+int (*save_setup)(CompressParam *param);
+void (*save_cleanup)(CompressParam *param);
+ssize_t (*compress_data)(CompressParam *param, size_t size);
+} MigrationCompressOps;
+
+typedef struct {
+int (*load_setup)(DecompressParam *param);
+void (*load_cleanup)(DecompressParam *param);
+int (*decompress_data)(DecompressParam *param, uint8_t *dest, size_t size);
+int (*check_len)(int len);
+} MigrationDecompressOps;
+
 static CompressParam *comp_param;
 static QemuThread *compress_threads;
+static MigrationCompressOps *compress_ops;
+static MigrationDecompressOps *decompress_ops;
 /* comp_done_cond is used to wake up the migration thread when
  * one of the compression threads has finished the compression.
  * comp_done_lock is used to co-work with comp_done_cond.
@@ -455,6 +475,157 @@ static QemuCond decomp_done_cond;
 
 static bool do_compress_ram_page(CompressParam *param, RAMBlock *block);
 
+static int zlib_save_setup(CompressParam *param)
+{
+if (deflateInit(>stream,
+migrate_compress_level()) != Z_OK) {
+return -1;
+}
+
+return 0;
+}
+
+static ssize_t zlib_compress_data(CompressParam *param, size_t size)
+{
+int err;
+uint8_t *dest = NULL;
+z_stream *stream = >stream;
+uint8_t *p = param->originbuf;
+QEMUFile *f = f = param->file;
+ssize_t blen = qemu_put_compress_start(f, );
+
+if (blen < compressBound(size)) {
+return -1;
+}
+
+err = deflateReset(stream);
+if (err != Z_OK) {
+return -1;
+}
+
+stream->avail_in = size;
+stream->next_in = p;
+stream->avail_out = blen;
+stream->next_out = dest;
+
+err = deflate(stream, Z_FINISH);
+if (err != Z_STREAM_END) {
+return -1;
+}
+
+blen = stream->next_out - dest;
+if (blen < 0) {
+return -1;
+}
+
+qemu_put_compress_end(f, blen);
+return blen + sizeof(int32_t);
+}
+
+static void zlib_save_cleanup(CompressParam *param)
+{
+deflateEnd(>stream);
+}
+
+static int zlib_load_setup(DecompressParam *param)
+{
+if (inflateInit(>stream) != Z_OK) {
+return -1;
+}
+
+return 0;
+}
+
+static int
+zlib_decompress_data(DecompressParam *param, uint8_t *dest, size_t size)
+{
+int err;
+
+z_stream *stream = >stream;
+
+err = inflateReset(stream);
+if (err != Z_OK) {
+return -1;
+}
+
+stream->avail_in = param->len;
+stream->next_in = param->compbuf;
+stream->avail_out = size;
+stream->next_out = dest;
+
+err = inflate(stream, Z_NO_FLUSH);
+if (err != Z_STREAM_END) {
+return -1;
+}
+
+return stream->total_out;
+}
+
+static void zlib_load_cleanup(DecompressParam *param)
+{
+inflateEnd(>stream);
+}
+
+static int zlib_check_len(int len)
+{
+return len < 0 || len > compressBound(TARGET_PAGE_SIZE);
+}
+
+static int set_compress_ops(void)
+{
+   compress_ops = g_new0(MigrationCompressOps, 1);
+
+switch (migrate_compress_method()) {
+case COMPRESS_METHOD_ZLIB:
+compress_ops->save_setup = zlib_save_setup;
+compress_ops->save_cleanup = zlib_save_cleanup;
+compress_ops->compress_data = zlib_compress_data;
+break;
+default:
+return -1;
+}
+
+return 0;
+}
+
+static int set_decompress_ops(void)
+{
+   decompress_ops = g_new0(MigrationDecompressOps, 1);
+
+switch (migrate_compress_method()) {
+case COMPRESS_METHOD_ZLIB:
+decompress_ops->load_setup = 

[PATCH v2 6/6] doc: Update multi-thread compression doc

[Zeyu Jin]

Modify the doc to fit the previous changes.

Signed-off-by: Zeyu Jin 
Signed-off-by: Ying Fang 
---
 docs/multi-thread-compression.txt | 31 ++-
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/docs/multi-thread-compression.txt 
b/docs/multi-thread-compression.txt
index bb88c6bdf1..d429963cb0 100644
--- a/docs/multi-thread-compression.txt
+++ b/docs/multi-thread-compression.txt
@@ -33,14 +33,15 @@ thread compression can be used to accelerate the 
compression process.
 
 The decompression speed of Zlib is at least 4 times as quick as
 compression, if the source and destination CPU have equal speed,
-keeping the compression thread count 4 times the decompression
-thread count can avoid resource waste.
+and you choose Zlib as compression method, keeping the compression
+thread count 4 times the decompression thread count can avoid resource waste.
 
 Compression level can be used to control the compression speed and the
-compression ratio. High compression ratio will take more time, level 0
-stands for no compression, level 1 stands for the best compression
-speed, and level 9 stands for the best compression ratio. Users can
-select a level number between 0 and 9.
+compression ratio. High compression ratio will take more time,
+level 1 stands for the best compression speed, and higher level means higher
+compression ration. For Zlib, users can select a level number between 0 and 9,
+where level 0 stands for no compression. For Zstd, users can select a
+level number between 1 and 22.
 
 
 When to use the multiple thread compression in live migration
@@ -116,16 +117,19 @@ to support the multiple thread compression migration:
 2. Activate compression on the source:
 {qemu} migrate_set_capability compress on
 
-3. Set the compression thread count on source:
+3. Set the compression method:
+{qemu} migrate_set_parameter compress_method zstd
+
+4. Set the compression thread count on source:
 {qemu} migrate_set_parameter compress_threads 12
 
-4. Set the compression level on the source:
+5. Set the compression level on the source:
 {qemu} migrate_set_parameter compress_level 1
 
-5. Set the decompression thread count on destination:
+6. Set the decompression thread count on destination:
 {qemu} migrate_set_parameter decompress_threads 3
 
-6. Start outgoing migration:
+7. Start outgoing migration:
 {qemu} migrate -d tcp:destination.host:
 {qemu} info migrate
 Capabilities: ... compress: on
@@ -136,6 +140,7 @@ The following are the default settings:
 compress_threads: 8
 decompress_threads: 2
 compress_level: 1 (which means best speed)
+compress_method: zlib
 
 So, only the first two steps are required to use the multiple
 thread compression in migration. You can do more if the default
@@ -143,7 +148,7 @@ settings are not appropriate.
 
 TODO
 
-Some faster (de)compression method such as LZ4 and Quicklz can help
-to reduce the CPU consumption when doing (de)compression. If using
-these faster (de)compression method, less (de)compression threads
+Comparing to Zlib, Some faster (de)compression method such as LZ4
+and Quicklz can help to reduce the CPU consumption when doing (de)compression.
+If using these faster (de)compression method, less (de)compression threads
 are needed when doing the migration.
-- 
2.27.0

[PATCH v2 1/6] migration: Add multi-thread compress method

[Zeyu Jin]

A multi-thread compress method parameter is added to hold the method we
are going to use. By default the 'zlib' method is used to maintain the
compatibility as before.

Signed-off-by: Zeyu Jin 
Signed-off-by: Ying Fang 
---
 hw/core/qdev-properties-system.c | 11 +++
 include/hw/qdev-properties.h |  4 
 migration/migration.c| 15 +++
 monitor/hmp-cmds.c   | 12 
 qapi/migration.json  | 26 +-
 5 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 9d80a07d26..a582721a7b 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -663,6 +663,17 @@ const PropertyInfo qdev_prop_multifd_compression = {
 .set_default_value = qdev_propinfo_set_default_value_enum,
 };
 
+/* --- CompressMethod --- */
+const PropertyInfo qdev_prop_compress_method = {
+.name = "CompressMethod",
+.description = "multi-thread compression method, "
+   "zlib",
+.enum_table = _lookup,
+.get = qdev_propinfo_get_enum,
+.set = qdev_propinfo_set_enum,
+.set_default_value = qdev_propinfo_set_default_value_enum,
+};
+
 /* --- Reserved Region --- */
 
 /*
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 4437450065..4a943f7e80 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -23,6 +23,7 @@ extern const PropertyInfo qdev_prop_macaddr;
 extern const PropertyInfo qdev_prop_reserved_region;
 extern const PropertyInfo qdev_prop_on_off_auto;
 extern const PropertyInfo qdev_prop_multifd_compression;
+extern const PropertyInfo qdev_prop_compress_method;
 extern const PropertyInfo qdev_prop_losttickpolicy;
 extern const PropertyInfo qdev_prop_blockdev_on_error;
 extern const PropertyInfo qdev_prop_bios_chs_trans;
@@ -193,6 +194,9 @@ extern const PropertyInfo qdev_prop_pcie_link_width;
 #define DEFINE_PROP_MULTIFD_COMPRESSION(_n, _s, _f, _d) \
 DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_multifd_compression, \
MultiFDCompression)
+#define DEFINE_PROP_COMPRESS_METHOD(_n, _s, _f, _d) \
+DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_compress_method, \
+   CompressMethod)
 #define DEFINE_PROP_LOSTTICKPOLICY(_n, _s, _f, _d) \
 DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_losttickpolicy, \
 LostTickPolicy)
diff --git a/migration/migration.c b/migration/migration.c
index 87a9b59f83..bfbe48cc74 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -83,6 +83,7 @@
 #define DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT 2
 /*0: means nocompress, 1: best speed, ... 9: best compress ratio */
 #define DEFAULT_MIGRATE_COMPRESS_LEVEL 1
+#define DEFAULT_MIGRATE_COMPRESS_METHOD COMPRESS_METHOD_ZLIB
 /* Define default autoconverge cpu throttle migration parameters */
 #define DEFAULT_MIGRATE_THROTTLE_TRIGGER_THRESHOLD 50
 #define DEFAULT_MIGRATE_CPU_THROTTLE_INITIAL 20
@@ -843,6 +844,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error 
**errp)
 params->compress_wait_thread = s->parameters.compress_wait_thread;
 params->has_decompress_threads = true;
 params->decompress_threads = s->parameters.decompress_threads;
+params->has_compress_method = true;
+params->compress_method = s->parameters.compress_method;
 params->has_throttle_trigger_threshold = true;
 params->throttle_trigger_threshold = 
s->parameters.throttle_trigger_threshold;
 params->has_cpu_throttle_initial = true;
@@ -1407,6 +1410,10 @@ static void 
migrate_params_test_apply(MigrateSetParameters *params,
 dest->decompress_threads = params->decompress_threads;
 }
 
+if (params->has_compress_method) {
+dest->compress_method = params->compress_method;
+}
+
 if (params->has_throttle_trigger_threshold) {
 dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
 }
@@ -1504,6 +1511,10 @@ static void migrate_params_apply(MigrateSetParameters 
*params, Error **errp)
 s->parameters.decompress_threads = params->decompress_threads;
 }
 
+if (params->has_compress_method) {
+s->parameters.compress_method = params->compress_method;
+}
+
 if (params->has_throttle_trigger_threshold) {
 s->parameters.throttle_trigger_threshold = 
params->throttle_trigger_threshold;
 }
@@ -3717,6 +3728,9 @@ static Property migration_properties[] = {
 DEFINE_PROP_UINT8("x-decompress-threads", MigrationState,
   parameters.decompress_threads,
   DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
+DEFINE_PROP_COMPRESS_METHOD("compress-method", MigrationState,
+  parameters.compress_method,
+  DEFAULT_MIGRATE_COMPRESS_METHOD),
 DEFINE_PROP_UINT8("x-throttle-trigger-threshold", MigrationState,
   

[PATCH v2 2/6] migration: Refactoring multi-thread compress migration

[Zeyu Jin]

Code refactor for the compression procedure which includes:

1. Move qemu_compress_data and qemu_put_compression_data from qemu-file.c to
ram.c, for the reason that most part of the code logical has nothing to do
with qemu-file. Besides, the decompression code is located at ram.c only.

2. Simplify the function input arguments for compression and decompression.
Wrap the input into the param structure which already exists. This change also
makes the function much more flexible for other compression methods.

Signed-off-by: Zeyu Jin 
Signed-off-by: Ying Fang 
---
 migration/qemu-file.c | 62 ++-
 migration/qemu-file.h |  4 +-
 migration/ram.c   | 86 ++-
 3 files changed, 75 insertions(+), 77 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index be21518c57..1efb667aa1 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -737,56 +737,6 @@ uint64_t qemu_get_be64(QEMUFile *f)
 return v;
 }
 
-/* return the size after compression, or negative value on error */
-static int qemu_compress_data(z_stream *stream, uint8_t *dest, size_t dest_len,
-  const uint8_t *source, size_t source_len)
-{
-int err;
-
-err = deflateReset(stream);
-if (err != Z_OK) {
-return -1;
-}
-
-stream->avail_in = source_len;
-stream->next_in = (uint8_t *)source;
-stream->avail_out = dest_len;
-stream->next_out = dest;
-
-err = deflate(stream, Z_FINISH);
-if (err != Z_STREAM_END) {
-return -1;
-}
-
-return stream->next_out - dest;
-}
-
-/* Compress size bytes of data start at p and store the compressed
- * data to the buffer of f.
- *
- * Since the file is dummy file with empty_ops, return -1 if f has no space to
- * save the compressed data.
- */
-ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream,
-  const uint8_t *p, size_t size)
-{
-ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
-
-if (blen < compressBound(size)) {
-return -1;
-}
-
-blen = qemu_compress_data(stream, f->buf + f->buf_index + sizeof(int32_t),
-  blen, p, size);
-if (blen < 0) {
-return -1;
-}
-
-qemu_put_be32(f, blen);
-add_buf_to_iovec(f, blen);
-return blen + sizeof(int32_t);
-}
-
 /* Put the data in the buffer of f_src to the buffer of f_des, and
  * then reset the buf_index of f_src to 0.
  */
@@ -846,3 +796,15 @@ void qemu_file_set_blocking(QEMUFile *f, bool block)
 f->ops->set_blocking(f->opaque, block, NULL);
 }
 }
+
+ssize_t qemu_put_compress_start(QEMUFile *f, uint8_t **dest_ptr)
+{
+*dest_ptr = f->buf + f->buf_index + sizeof(int32_t);
+return IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
+}
+
+void qemu_put_compress_end(QEMUFile *f, unsigned int v)
+{
+qemu_put_be32(f, v);
+add_buf_to_iovec(f, v);
+}
diff --git a/migration/qemu-file.h b/migration/qemu-file.h
index a9b6d6ccb7..1ac1566460 100644
--- a/migration/qemu-file.h
+++ b/migration/qemu-file.h
@@ -138,8 +138,6 @@ bool qemu_file_is_writable(QEMUFile *f);
 
 size_t qemu_peek_buffer(QEMUFile *f, uint8_t **buf, size_t size, size_t 
offset);
 size_t qemu_get_buffer_in_place(QEMUFile *f, uint8_t **buf, size_t size);
-ssize_t qemu_put_compression_data(QEMUFile *f, z_stream *stream,
-  const uint8_t *p, size_t size);
 int qemu_put_qemu_file(QEMUFile *f_des, QEMUFile *f_src);
 
 /*
@@ -166,6 +164,8 @@ void ram_control_before_iterate(QEMUFile *f, uint64_t 
flags);
 void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
 void ram_control_load_hook(QEMUFile *f, uint64_t flags, void *data);
 
+ssize_t qemu_put_compress_start(QEMUFile *f, uint8_t **dest_ptr);
+void qemu_put_compress_end(QEMUFile *f, unsigned int v);
 /* Whenever this is found in the data stream, the flags
  * will be passed to ram_control_load_hook in the incoming-migration
  * side. This lets before_ram_iterate/after_ram_iterate add
diff --git a/migration/ram.c b/migration/ram.c
index 7811cde643..1818a56314 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -453,27 +453,22 @@ static QemuThread *decompress_threads;
 static QemuMutex decomp_done_lock;
 static QemuCond decomp_done_cond;
 
-static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock 
*block,
- ram_addr_t offset, uint8_t *source_buf);
+static bool do_compress_ram_page(CompressParam *param, RAMBlock *block);
 
 static void *do_data_compress(void *opaque)
 {
 CompressParam *param = opaque;
 RAMBlock *block;
-ram_addr_t offset;
 bool zero_page;
 
 qemu_mutex_lock(>mutex);
 while (!param->quit) {
 if (param->block) {
 block = param->block;
-offset = param->offset;
 param->block = NULL;
 qemu_mutex_unlock(>mutex);
 
-zero_page = 

[PATCH v2 0/6]

[Zeyu Jin]

Currently we have both multi-thread compression and multifd to optimize
live migration in Qemu. Mulit-thread compression deals with the situation
where network bandwith is limited but cpu resource adequate. Multifd instead
aims to take full advantage of network bandwith. Moreover it supports both
zlib and zstd compression on each channel.

In this patch series, we did some code refactoring on multi-thread compression
live migration and bring zstd compression method support for it.

Below is the test result of multi-thread compression live migration
with different compress methods. Test result shows that zstd outperforms
zlib by about 70%.

 Migration Configuration:
 Guest 8U 32G
 compress-threads   8
 decompress-threads 2
 compress-level 1
 bandwidth-limit 100Mbps

 Test Result:
 +-+--+-+
 |  compress method|   zlib   |zstd |
 +-+--+-+
 |  total time (ms)|   75256  |44187|
 +-+--+-+
 |  downtime(ms)   |   128|81   |
 +-+--+-+
 |  transferred ram(kB)|   1576866|736117   |
 +-+--+-+
 |  throughput(mbps)   |   172.06 |137.16   |
 +-+--+-+
 |  total ram(kB)  |   33685952   |33685952 |
 +-+--+-+

Zeyu Jin (6):
  migration: Add multi-thread compress method
  migration: Refactoring multi-thread compress migration
  migration: Add multi-thread compress ops
  migration: Add zstd support in multi-thread compression
  migration: Add compress_level sanity check
  doc: Update multi-thread compression doc

 docs/multi-thread-compression.txt |  31 ++-
 hw/core/qdev-properties-system.c  |  11 +
 include/hw/qdev-properties.h  |   4 +
 migration/migration.c |  56 -
 migration/migration.h |   1 +
 migration/qemu-file.c |  62 +
 migration/qemu-file.h |   4 +-
 migration/ram.c   | 381 +-
 monitor/hmp-cmds.c|  12 +
 qapi/migration.json   |  26 +-
 10 files changed, 465 insertions(+), 123 deletions(-)

-- 
2.27.0

Re: [PATCH 1/2] keyval: accept escaped commas in implied option

[Paolo Bonzini]

[huge snip]

On 27/11/20 09:38, Markus Armbruster wrote:

The suboptimal error message is due to the way I coded the parser, not
due to the grammar.


Small digression: a different grammar influences how the parser is 
written.  You coded the parser like this because you thought of implied 
options as "key without ="; instead I thought of them as "value not 
preceded by key=".




  --nbd key=val,=,fob=

master:   Invalid parameter ''
your patch:   Expected parameter before '='

Improvement, but which '='?  Possibly better:

  Expected parameter before '=,fob='


Yup, easy.


   --nbd .key=

 master:   Invalid parameter '..key'
 your patch:   same

 Better, I think:

   Name expected before '..key'

   Odd: if I omit the '=', your patch's message often changes to

   Expected '=' after parameter ...

   This means the parser reports a non-first syntax error.  Parsing
   smell, I'm afraid :)


Nah, just lazy cut-and-paste of the existing error message.  I should 
rename that error to something "No implicit parameter name for '.key'" 
(again, different grammar -> different parser -> different error).  That 
error message actually makes sense: "--object .key" would create an 
object of type ".key" both without or with these changes.



* Invalid key fragment

   --nbd key.1a.b=

 master:   Invalid parameter 'key.1a.b'
 your patch:   same

 Slightly better, I think:

   'key.1a' is not a valid parameter name


Or just "Invalid parameter '1a'".  I'm not going to do that in v2 
though, since parameter parsing is not being



I believe there are two, maybe three reasons for this series:

1. Support ',' in values with an implicit keys.

2. Improve error reporting.

3. Maybe nicer code.

1. is a feature without a known use.


Breaking news: there is actually a use.  I should have pointed out in 
the commit message, but I didn't realize at the time, that this patch 
fixes device-introspect-test once device_add is switched to keyval-based 
parsing.  And why is that?  Because even though SUNW,fdtwo is not 
user-creatable, you can still do "device_add SUNW,,fdtwo,help".  It even 
works from the command line:


$ qemu-system-sparc -device SUNW,,fdtwo,help
SUNW,fdtwo options:
  drive=- Node name or ID of a block device to use as 
a backend
  fallback= - FDC drive type, 144/288/120/none/auto 
(default: "144")

  ...

This invocation is useful (for some value of useful) to see which 
properties you can pass with -global.  So there *is* a valid (for some 
value of valid) use of escaped commas in implied options.  It can be 
fixed with deprecation etc. but that would be a more complicated 
endeavor than just adjusting keyval.



2. can be had with much less churn
(I'm ready to back that up with a patch).  Since I haven't looked at
PATCH 2, I'm reserving judgement on 3.


FWIW I think this patch is already an improvement in code niceness, 
though I accept that it's in the eye of the beholder.


Paolo

Re: [PATCH 0/1] net: Fix handling of id in netdev_add and netdev_del

[Markus Armbruster]

Jason Wang  writes:

> On 2020/11/25 下午6:02, Markus Armbruster wrote:
>> This is a regression fix, but the regression is already in 5.0.  I
>> think it's too late for 5.2.  If I'm right, then the issue should be
>> documented in the release notes (I can do that).
>
>
> Please do that and I've queued this for the next release.

Done.  Thanks!

Re: [PATCH] virtfs-proxy-helper: Fix a resource leak in main()

[Greg Kurz]

On Thu, 26 Nov 2020 19:44:24 +0100
Christian Schoenebeck  wrote:

[...]
> > The only justification that'd deserve to be in the changelog of
> > such a patch is something like "because this is good practice
> > to rollback in case code moves to another function than main()".
> 
> Well, the actual motivation was rather a pragmatic one: to shut up a 
> sanitizer's false positive, which I can understand.
> 

Yes, this should also be mentioned in the changelog.

> Another option would be using a global variable for the fd instead of a 
> temporary on stack. That should shut up the sanitizer as well and would not 
> introduce change to the program flow.
> 

Using the same sock variable for an fd that is either passed to us
or that we create is a very poor programming choice actually... :(

So if the motivation is just to make "Euler Robot" happy and this
can be addressed as you suggest, I personally prefer that rather
than piling up fixes on broken code.

> I leave that up to Greg to decide whether or not to handle this. I'm 
> Switzerland on this one.
> 

This won't go into QEMU 5.2 anyway since we only merge fixes for
critical bugs or regressions at this point. No hurry to decide
anything now :)

> Best regards,
> Christian Schoenebeck
> 
> 

Re: [PATCH v2 2/6] accel: accel_available() function

[Claudio Fontana]

On 11/26/20 10:48 PM, Eduardo Habkost wrote:
> On Thu, Nov 26, 2020 at 10:06:03PM +0100, Claudio Fontana wrote:
>> On 11/26/20 3:25 PM, Paolo Bonzini wrote:
>>> On 26/11/20 15:13, Claudio Fontana wrote:
 One option I see is simply to document the behavior where
 accel_available() is declared in accel.h (ie do not use in fast
 path), as well as in accel_find() actually, so that both accel_find()
 and accel_available() are avoided in fast path and avoid being called
 frequently at runtime.

 Another option could be to remove the allocation completely, and use
 for example accel_find(ACCEL_CLASS_NAME("tcg")), or another option
 again would be to remove the allocation and use either a fixed buffer
 + snprintf, or alloca -like builtin code to use the stack, ...

 Not a big deal, but with a general utility and short name like
 accel_available(name) it might be tempting to use this more in the
 future?
>>>
>>> I think it's just that the usecase is not that common.  "Is this 
>>> accelerator compiled in the binary" is not something you need after 
>>> startup (or if querying the monitor).
>>>
>>> Paolo
>>>
>>>
>>
>> A script that repeatedly uses the QMP interface to query for
>> the status could generate fragmentation this way I think.
> 
> Is this a problem?  Today, execution of a "query-kvm" command
> calls g_malloc() 37 times.
> 

Not ideal in my view, but not the end of the world either.

Ciao,

Claudio

Re: [RFC v6 10/11] accel: introduce AccelCPUClass extending CPUClass

[Claudio Fontana]

On 11/27/20 7:21 AM, Paolo Bonzini wrote:
> On 26/11/20 23:32, Claudio Fontana wrote:
>> +if (acc) {
>> +object_class_foreach(accel_init_cpu_int_aux, cpu_type, false, acc);
>> +}
> 
> Any reason to do it for cpu_type only, rather than for all subclasses of 
> CPU_RESOLVING_TYPE?  This would remove the cpu_type argument to 
> accel_init_cpu_interfaces and accel_init_interfaces.
> 
> Otherwise I haven't done a careful review yet but it looks very nice, 
> thanks!
> 
> Paolo
> 

Hi Paolo,

yes, I thought to pass cpu_type in order to set the interface only for the cpu 
that is actually used,
instead of looping over all cpu models, just to be a bit quicker, but both 
things should work.

Ciao,

Claudio

Re: [PATCH 1/2] keyval: accept escaped commas in implied option

[Markus Armbruster]

Paolo Bonzini  writes:

> This is used with the weirdly-named device "SUNFD,two", so accepting it

"SUNW,fdtwo"

> is also a preparatory step towards keyval-ifying -device and the
> device_add monitor command.

Not quite.  Device "SUNW,fdtwo" has user_creatable = false (it's a
sysbus device), and therefore isn't available with -device or
device_add.

There are more device names containing comma, but only one is available
with -device or device_add: xlnx,zynqmp-pmu-soc.  I doubt it makes sense
there.

Parameter values with comma need special treatment before and after the
patch.  Before the patch, you have to use the unsugared form and double
the commas.  After the patch, that still works, but you can now *also*
use the sugared form and double the commas.  Not much of an improvement,
I'm afraid.

Of course, we aren't really after improvement, we're after switching to
keyval_parse() with fewer compatiblity issues.  But this issue only
exists where values with comma are valid.

None of the QemuOpts I looked at take such values, except for -device
xlnx,zynqmp-pmu-soc, which I believe to be useless.  If an actual use
exists, I missed it.

Permit me to digress: we should kill the anti-social device names
regardless of this patch.

I want device names to conform to QAPI naming rules for enum values.  If
we ever do QAPI-schema-based device configuration, "driver" changes from
str to enum.  Even if we're ready to reject QAPI-schema-based device
configuration as impractical forever, there's still a consistency
argument: names of things should conform to a common set of rules.

This applies to all implied parameters with an emumeration-like value.
I'm not aware of other offenders, though.

>  But in general it is an unexpected wart
> of the keyval syntax and leads to suboptimal errors compared to QemuOpts:
>
>   $ ./qemu-system-x86_64 -object foo,,bar,id=obj
>   qemu-system-x86_64: -object foo,,bar,id=obj: invalid object type: foo,bar
>   $ storage-daemon/qemu-storage-daemon --object foo,,bar,id=obj
>   qemu-storage-daemon: Invalid parameter ''

The suboptimal error message is due to the way I coded the parser, not
due to the grammar.

> To implement this, the flow of the parser is changed to first unescape
> everything up to the next comma or equal sign.  This is done in a
> new function keyval_fetch_string for both the key and value part.
> Keys therefore are now parsed in unescaped form, but this makes no
> difference in practice because a comma is an invalid character for a
> QAPI name.  Thus keys with a comma in them are rejected anyway, as
> demonstrated by the new testcase.
>
> As a side effect of the new code, parse errors are slightly improved as
> well: "Invalid parameter ''" becomes "Expected parameter before '='"
> when keyval is fed a string starting with an equal sign.

Yes, my parse errors are less than friendly.  Let's review some of them.
I'm testing with

$ qemu-storage-daemon --nbd $ARG

because that one doesn't have an implied key, which permits slightly
simpler $ARG.

* Empty key

  --nbd ,

master:   Invalid parameter ''
your patch:   Expected parameter before ','

Improvement.

  --nbd key=val,=,fob=

master:   Invalid parameter ''
your patch:   Expected parameter before '='

Improvement, but which '='?  Possibly better:

  Expected parameter before '=,fob='

* Empty key fragment

  --nbd key..=

master:   Invalid parameter 'key..'
your patch:   same

Slightly better, I think:

  Name or number expected after 'key.'

  --nbd .key=

master:   Invalid parameter '..key'
your patch:   same

Better, I think:

  Name expected before '..key'

  Odd: if I omit the '=', your patch's message often changes to

  Expected '=' after parameter ...

  This means the parser reports a non-first syntax error.  Parsing
  smell, I'm afraid :)

* Invalid key fragment

  --nbd _=

master:   Invalid parameter '_'
your patch:   same

  --nbd key.1a.b=

master:   Invalid parameter 'key.1a.b'
your patch:   same

Slightly better, I think:

  'key.1a' is not a valid parameter name

  --ndb anti,,social,,key=

master:   Expected '=' after parameter 'anti'
your patch:   Invalid parameter 'anti,social,key'

The new error message shows the *unescaped* string.  Okay.

> The slightly baroque interface of keyval_fetch_string lets me keep the
> key parsing loop mostly untouched.  It is simplified in the next patch,
> however.
>
> Signed-off-by: Paolo Bonzini 

I believe there are two, maybe three reasons for this series:

1. Support ',' in values with an implicit keys.

2. Improve error reporting.

3. Maybe nicer code.

1. is a feature without a known use.  2. can be had with much less churn
(I'm ready to back that up with a patch).  Since I haven't looked at
PATCH 2, I'm reserving judgement on 3.

[PATCH v2] qga/commands-posix: Send CCW address on s390x with the fsinfo data

[Thomas Huth]

We need the CCW address on the libvirt side to correctly identify
the disk, so add this information to the GuestDiskAddress on s390x.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1755075
Signed-off-by: Thomas Huth 
---
 v2: Add missing comment about "subchno" (thanks to Cornelia for spotting this)

 The libirt part of this fix can be found here:
 https://www.redhat.com/archives/libvir-list/2020-November/msg01455.html

 qga/commands-posix.c | 34 ++
 qga/qapi-schema.json | 20 +++-
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c089e38120..5aa5eff84f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1029,6 +1029,38 @@ static bool build_guest_fsinfo_for_nonpci_virtio(char 
const *syspath,
 return true;
 }
 
+/*
+ * Store disk device info for CCW devices (s390x channel I/O devices).
+ * Returns true if information has been stored, or false for failure.
+ */
+static bool build_guest_fsinfo_for_ccw_dev(char const *syspath,
+   GuestDiskAddress *disk,
+   Error **errp)
+{
+unsigned int cssid, ssid, subchno, devno;
+char *p;
+
+p = strstr(syspath, "/devices/css");
+if (!p || sscanf(p + 12, "%*x/%x.%x.%x/%*x.%*x.%x/",
+ , , , ) < 4) {
+g_debug("could not parse ccw device sysfs path: %s", syspath);
+return false;
+}
+
+disk->has_ccw_address = true;
+disk->ccw_address = g_new0(GuestCCWAddress, 1);
+disk->ccw_address->cssid = cssid;
+disk->ccw_address->ssid = ssid;
+disk->ccw_address->subchno = subchno;
+disk->ccw_address->devno = devno;
+
+if (strstr(p, "/virtio")) {
+build_guest_fsinfo_for_nonpci_virtio(syspath, disk, errp);
+}
+
+return true;
+}
+
 /* Store disk device info specified by @sysfs into @fs */
 static void build_guest_fsinfo_for_real_device(char const *syspath,
GuestFilesystemInfo *fs,
@@ -1081,6 +1113,8 @@ static void build_guest_fsinfo_for_real_device(char const 
*syspath,
 
 if (strstr(syspath, "/devices/pci")) {
 has_hwinf = build_guest_fsinfo_for_pci_dev(syspath, disk, errp);
+} else if (strstr(syspath, "/devices/css")) {
+has_hwinf = build_guest_fsinfo_for_ccw_dev(syspath, disk, errp);
 } else if (strstr(syspath, "/virtio")) {
 has_hwinf = build_guest_fsinfo_for_nonpci_virtio(syspath, disk, errp);
 } else {
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index 3b3d1d0bd9..9a82b7e952 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -846,6 +846,22 @@
   'data': {'domain': 'int', 'bus': 'int',
'slot': 'int', 'function': 'int'} }
 
+##
+# @GuestCCWAddress:
+#
+# @cssid: channel subsystem image id
+# @ssid: subchannel set id
+# @subchno: subchannel number
+# @devno: device number
+#
+# Since: 6.0
+##
+{ 'struct': 'GuestCCWAddress',
+  'data': {'cssid': 'int',
+   'ssid': 'int',
+   'subchno': 'int',
+   'devno': 'int'} }
+
 ##
 # @GuestDiskAddress:
 #
@@ -856,6 +872,7 @@
 # @unit: unit id
 # @serial: serial number (since: 3.1)
 # @dev: device node (POSIX) or device UNC (Windows) (since: 3.1)
+# @ccw-address: CCW address on s390x (since: 6.0)
 #
 # Since: 2.2
 ##
@@ -863,7 +880,8 @@
   'data': {'pci-controller': 'GuestPCIAddress',
'bus-type': 'GuestDiskBusType',
'bus': 'int', 'target': 'int', 'unit': 'int',
-   '*serial': 'str', '*dev': 'str'} }
+   '*serial': 'str', '*dev': 'str',
+   '*ccw-address': 'GuestCCWAddress'} }
 
 ##
 # @GuestDiskInfo:
-- 
2.18.4

Re: [PATCH v4 0/6] UFFD write-tracking migration/snapshots

[Andrey Gruzdev]

On 26.11.2020 18:47, Peter Krempa wrote:

On Thu, Nov 26, 2020 at 18:17:28 +0300, Andrey Gruzdev via wrote:

This patch series is a kind of 'rethinking' of Denis Plotnikov's ideas he's
implemented in his series '[PATCH v0 0/4] migration: add background snapshot'.

Currently the only way to make (external) live VM snapshot is using existing
dirty page logging migration mechanism. The main problem is that it tends to
produce a lot of page duplicates while running VM goes on updating already
saved pages. That leads to the fact that vmstate image size is commonly several
times bigger then non-zero part of virtual machine's RSS. Time required to
converge RAM migration and the size of snapshot image severely depend on the
guest memory write rate, sometimes resulting in unacceptably long snapshot
creation time and huge image size.

This series propose a way to solve the aforementioned problems. This is done
by using different RAM migration mechanism based on UFFD write protection
management introduced in v5.7 kernel. The migration strategy is to 'freeze'
guest RAM content using write-protection and iteratively release protection
for memory ranges that have already been saved to the migration stream.
At the same time we read in pending UFFD write fault events and save those
pages out-of-order with higher priority.


This sounds amazing! Based on your description I assume that the final
memory image contains state image from the beginning of the migration.

This would make it desirable for the 'migrate' qmp command to be used as
part of a 'transaction' qmp command so that we can do an instant disk
and memory snapshot without any extraneous pausing of the VM.

I'll have a look at using this mechanism in libvirt natively.



Correct, the final image contains state at the beginning of migration.

So far, if I'm not missing something about libvirt, for external 
snapshot creation it performs a sequence like that: 
migrate(fd)->transaction(blockdev-snapshot-all)->cont.


So, in case 'background-snapshot' capability is enabled, the sequence 
would change to:

stop->transaction(blockdev-snapshot-all)->migrate(fd).
With background snapshot migration it will finish with VM running so 
there's not need to 'cont' here.


Thanks, I'd appreciate a lot if you look how to integrate the new 
mechanism to libvirt!


--
Andrey Gruzdev, Principal Engineer
Virtuozzo GmbH  +7-903-247-6397
virtuzzo.com