Re: [PATCH v29 17/17] qapi: Add VFIO devices migration stats in Migration stats

From: Thomas Huth <thuth@redhat.com>
To: Kirti Wankhede <kwankhede@nvidia.com>,
	alex.williamson@redhat.com, cjia@nvidia.com, dgilbert@redhat.com,
	quintela@redhat.com
Cc: zhi.wang.linux@gmail.com, aik@ozlabs.ru,
	Zhengxiao.zx@Alibaba-inc.com, shuangtai.tst@alibaba-inc.com,
	qemu-devel@nongnu.org, peterx@redhat.com, eauger@redhat.com,
	artemp@nvidia.com, yi.l.liu@intel.com, eskultet@redhat.com,
	ziye.yang@intel.com, armbru@redhat.com, mlevitsk@redhat.com,
	pasic@linux.ibm.com, felipe@nutanix.com, Ken.Xue@amd.com,
	mcrossley@nvidia.com, kevin.tian@intel.com, yan.y.zhao@intel.com,
	changpeng.liu@intel.com, cohuck@redhat.com, zhi.a.wang@intel.com,
	jonathan.davies@nutanix.com, pbonzini@redhat.com,
	dnigam@nvidia.com
Subject: Re: [PATCH v29 17/17] qapi: Add VFIO devices migration stats in Migration stats
Date: Mon, 15 Mar 2021 14:08:13 +0100	[thread overview]
Message-ID: <3ba4153d-7a81-c9e5-709d-60a10683b6ef@redhat.com> (raw)
In-Reply-To: <1603704987-20977-18-git-send-email-kwankhede@nvidia.com>

On 26/10/2020 10.36, Kirti Wankhede wrote:
> Added amount of bytes transferred to the VM at destination by all VFIO
> devices
> 
> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>   hw/vfio/common.c              | 19 +++++++++++++++++++
>   hw/vfio/migration.c           |  9 +++++++++
>   include/hw/vfio/vfio-common.h |  3 +++
>   migration/migration.c         | 17 +++++++++++++++++
>   monitor/hmp-cmds.c            |  6 ++++++
>   qapi/migration.json           | 17 +++++++++++++++++
>   6 files changed, 71 insertions(+)
[...]
> diff --git a/migration/migration.c b/migration/migration.c
> index 0575ecb37953..995ccd96a774 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -57,6 +57,10 @@
>   #include "qemu/queue.h"
>   #include "multifd.h"
>   
> +#ifdef CONFIG_VFIO
> +#include "hw/vfio/vfio-common.h"
> +#endif
> +
>   #define MAX_THROTTLE  (128 << 20)      /* Migration transfer speed throttling */
>   
>   /* Amount of time to allocate to each "chunk" of bandwidth-throttled
> @@ -1002,6 +1006,17 @@ static void populate_disk_info(MigrationInfo *info)
>       }
>   }
>   
> +static void populate_vfio_info(MigrationInfo *info)
> +{
> +#ifdef CONFIG_VFIO
> +    if (vfio_mig_active()) {
> +        info->has_vfio = true;
> +        info->vfio = g_malloc0(sizeof(*info->vfio));
> +        info->vfio->transferred = vfio_mig_bytes_transferred();
> +    }
> +#endif
> +}

  Hi!

I'm afraid, but this #ifdef CONFIG_VFIO here likely does not work as 
expected: migration/migration.c is common code, i.e. it is compiled only 
once for all targets. But CONFIG_VFIO is a target-specific config switch, so 
this is only defined properly for target specific .c files. So depending on 
which target has been compiled first, the code might be included or not for 
all the other targets, no matter whether they have VFIO or not.
To fix this issue, I think it's likely best to move the function into a new 
file instead and include that via specific_ss.add() in the meson.build file.

  Thomas