Re: [PATCH v26 07/17] vfio: Register SaveVMHandlers for VFIO device

From: Cornelia Huck <cohuck@redhat.com>
To: Kirti Wankhede <kwankhede@nvidia.com>
Cc: cjia@nvidia.com, aik@ozlabs.ru, Zhengxiao.zx@Alibaba-inc.com,
	shuangtai.tst@alibaba-inc.com, qemu-devel@nongnu.org,
	peterx@redhat.com, eauger@redhat.com, yi.l.liu@intel.com,
	quintela@redhat.com, ziye.yang@intel.com, armbru@redhat.com,
	mlevitsk@redhat.com, pasic@linux.ibm.com, felipe@nutanix.com,
	zhi.a.wang@intel.com, kevin.tian@intel.com, yan.y.zhao@intel.com,
	dgilbert@redhat.com, alex.williamson@redhat.com,
	changpeng.liu@intel.com, eskultet@redhat.com, Ken.Xue@amd.com,
	jonathan.davies@nutanix.com, pbonzini@redhat.com
Subject: Re: [PATCH v26 07/17] vfio: Register SaveVMHandlers for VFIO device
Date: Tue, 20 Oct 2020 17:51:58 +0200	[thread overview]
Message-ID: <20201020175158.784e92ca.cohuck@redhat.com> (raw)
In-Reply-To: <34146d7a-6b38-132d-ab2e-90f5d8b71f9e@nvidia.com>

On Mon, 19 Oct 2020 02:25:28 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 9/25/2020 5:23 PM, Cornelia Huck wrote:
> > On Wed, 23 Sep 2020 04:54:09 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> Define flags to be used as delimeter in migration file stream.
> >> Added .save_setup and .save_cleanup functions. Mapped & unmapped migration
> >> region from these functions at source during saving or pre-copy phase.
> >> Set VFIO device state depending on VM's state. During live migration, VM is
> >> running when .save_setup is called, _SAVING | _RUNNING state is set for VFIO
> >> device. During save-restore, VM is paused, _SAVING state is set for VFIO device.
> >>
> >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> >> Reviewed-by: Neo Jia <cjia@nvidia.com>
> >> ---
> >>   hw/vfio/migration.c  | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>   hw/vfio/trace-events |  2 ++
> >>   2 files changed, 93 insertions(+)
> >>  
> > 
> > (...)
> >   
> >> +/*
> >> + * Flags used as delimiter:
> >> + * 0xffffffff => MSB 32-bit all 1s
> >> + * 0xef10     => emulated (virtual) function IO  
> > 
> > Where is this value coming from?
> >   
> 
> Delimiter flags should be unique and this is a magic number that 
> represents (e)mulated (f)unction (10) representing IO.
> 
> >> + * 0x0000     => 16-bits reserved for flags
> >> + */
> >> +#define VFIO_MIG_FLAG_END_OF_STATE      (0xffffffffef100001ULL)
> >> +#define VFIO_MIG_FLAG_DEV_CONFIG_STATE  (0xffffffffef100002ULL)
> >> +#define VFIO_MIG_FLAG_DEV_SETUP_STATE   (0xffffffffef100003ULL)
> >> +#define VFIO_MIG_FLAG_DEV_DATA_STATE    (0xffffffffef100004ULL)  
> > 
> > I think we need some more documentation what these values mean and how
> > they are used. From reading ahead a bit, it seems there is always
> > supposed to be a pair of DEV_*_STATE and END_OF_STATE framing some kind
> > of data?
> >   
> 
> Adding comment as below, hope it helps.
> 
> /*
>   * Flags used as delimiter for VFIO devices should be unique in 
> migration stream

Maybe

"Flags to be used as unique delimiters for VFIO devices in the
migration stream" ?

>   * These flags are composed as:
>   * 0xffffffff => MSB 32-bit all 1s
>   * 0xef10     => Magic ID, represents emulated (virtual) function IO
>   * 0x0000     => 16-bits reserved for flags
>   *
>   * Flags _DEV_CONFIG_STATE, _DEV_SETUP_STATE and _DEV_DATA_STATE marks 
> start of
>   * respective states in migration stream.
>   * FLAG _END_OF_STATE indicates end of current state, state could be any
>   * of above states.
>   */

"The beginning of state information is marked by _DEV_CONFIG_STATE,
_DEV_SETUP_STATE, or _DEV_DATA_STATE, respectively. The end of a
certain state information is marked by _END_OF_STATE." ?

> 
> Thanks,
> Kirti
> 