netdev - RE: [PATCH V7 mlx5-next 08/15] vfio: Define device migration protocol v2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BN9PR11MB5276B8754BA82E94CF288F8C8C349@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Tue, 15 Feb 2022 08:04:27 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Yishai Hadas <yishaih@...dia.com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "jgg@...dia.com" <jgg@...dia.com>,
        "saeedm@...dia.com" <saeedm@...dia.com>
CC:     "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "leonro@...dia.com" <leonro@...dia.com>,
        "kwankhede@...dia.com" <kwankhede@...dia.com>,
        "mgurtovoy@...dia.com" <mgurtovoy@...dia.com>,
        "maorg@...dia.com" <maorg@...dia.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "shameerali.kolothum.thodi@...wei.com" 
        <shameerali.kolothum.thodi@...wei.com>
Subject: RE: [PATCH V7 mlx5-next 08/15] vfio: Define device migration protocol
 v2

> From: Yishai Hadas <yishaih@...dia.com>
> Sent: Tuesday, February 8, 2022 1:22 AM
> 
> +static int vfio_ioctl_device_feature_migration(struct vfio_device *device,
> +					       u32 flags, void __user *arg,
> +					       size_t argsz)
> +{
> +	struct vfio_device_feature_migration mig = {
> +		.flags = VFIO_MIGRATION_STOP_COPY,
> +	};
> +	int ret;
> +
> +	if (!device->ops->migration_set_state)
> +		return -ENOTTY;

Miss a check on migration_get_state, as done in last function.

> + * @migration_set_state: Optional callback to change the migration state for
> + *         devices that support migration. The returned FD is used for data
> + *         transfer according to the FSM definition. The driver is responsible
> + *         to ensure that FD is isolated whenever the migration FSM leaves a
> + *         data transfer state or before close_device() returns.

didn't understand the meaning of 'isolated' here.

> +#define VFIO_DEVICE_STATE_V1_STOP      (0)
> +#define VFIO_DEVICE_STATE_V1_RUNNING   (1 << 0)
> +#define VFIO_DEVICE_STATE_V1_SAVING    (1 << 1)
> +#define VFIO_DEVICE_STATE_V1_RESUMING  (1 << 2)
> +#define VFIO_DEVICE_STATE_MASK      (VFIO_DEVICE_STATE_V1_RUNNING
> | \
> +				     VFIO_DEVICE_STATE_V1_SAVING |  \
> +				     VFIO_DEVICE_STATE_V1_RESUMING)

Does it make sense to also add 'V1' to MASK and also following macros
given their names are general?

  #define VFIO_DEVICE_STATE_VALID(state) \
  #define VFIO_DEVICE_STATE_IS_ERROR(state) \
  #define VFIO_DEVICE_STATE_SET_ERROR(state) \

It certainly implies more changes to v1 code but readability can be
slightly improved.

> +/*
> + * Indicates the device can support the migration API. See enum

call it V2? Not necessary to add V2 in code but worthy of a clarification
in comment.

> + * vfio_device_mig_state for details. If present flags must be non-zero and
> + * VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE is supported.
> + *
> + * VFIO_MIGRATION_STOP_COPY means that RUNNING, STOP, STOP_COPY
> and
> + * RESUMING are supported.
> + */

Not aligned with other places where 5 states are mentioned. Better add
ERROR here.

> + *
> + * RUNNING -> STOP
> + * STOP_COPY -> STOP
> + *   While in STOP the device must stop the operation of the device. The
> + *   device must not generate interrupts, DMA, or advance its internal
> + *   state. When stopped the device and kernel migration driver must accept
> + *   and respond to interaction to support external subsystems in the STOP
> + *   state, for example PCI MSI-X and PCI config pace. Failure by the user to
> + *   restrict device access while in STOP must not result in error conditions
> + *   outside the user context (ex. host system faults).

Right above the STOP state is defined as:

       *  STOP - The device does not change the internal or external state

'external state' I assume means P2P activities. For consistency it is clearer
to also say something about external state in above paragraph.

> + *
> + *   The STOP_COPY arc will terminate a data transfer session.

remove 'will'

> + *
> + * STOP -> STOP_COPY
> + *   This arc begin the process of saving the device state and will return a
> + *   new data_fd.
> + *
> + *   While in the STOP_COPY state the device has the same behavior as STOP
> + *   with the addition that the data transfers session continues to stream the
> + *   migration state. End of stream on the FD indicates the entire device
> + *   state has been transferred.
> + *
> + *   The user should take steps to restrict access to vfio device regions while
> + *   the device is in STOP_COPY or risk corruption of the device migration
> data
> + *   stream.

Restricting access has been explained in the to-STOP arcs and it is stated 
that while in STOP_COPY the device has the same behavior as STOP. So 
I think the last paragraph is possibly not required.

> + *
> + * STOP -> RESUMING
> + *   Entering the RESUMING state starts a process of restoring the device
> + *   state and will return a new data_fd. The data stream fed into the
> data_fd
> + *   should be taken from the data transfer output of the saving group states

No definition of 'group state' (maybe introduced in a later patch?)

> + *   from a compatible device. The migration driver may alter/reset the
> + *   internal device state for this arc if required to prepare the device to
> + *   receive the migration data.
> + *

Thanks
Kevin