Messages in this thread | | | Subject | Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity | From | Max Gurtovoy <> | Date | Wed, 29 Sep 2021 18:28:44 +0300 |
| |
On 9/29/2021 6:17 PM, Alex Williamson wrote: > On Wed, 29 Sep 2021 17:36:59 +0300 > Max Gurtovoy <mgurtovoy@nvidia.com> wrote: > >> On 9/29/2021 4:50 PM, Alex Williamson wrote: >>> On Wed, 29 Sep 2021 16:26:55 +0300 >>> Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>> >>>> On 9/29/2021 3:35 PM, Alex Williamson wrote: >>>>> On Wed, 29 Sep 2021 13:44:10 +0300 >>>>> Max Gurtovoy <mgurtovoy@nvidia.com> wrote: >>>>> >>>>>> On 9/28/2021 2:12 AM, Jason Gunthorpe wrote: >>>>>>> On Mon, Sep 27, 2021 at 04:46:48PM -0600, Alex Williamson wrote: >>>>>>>>> + enum { MAX_STATE = VFIO_DEVICE_STATE_RESUMING }; >>>>>>>>> + static const u8 vfio_from_state_table[MAX_STATE + 1][MAX_STATE + 1] = { >>>>>>>>> + [VFIO_DEVICE_STATE_STOP] = { >>>>>>>>> + [VFIO_DEVICE_STATE_RUNNING] = 1, >>>>>>>>> + [VFIO_DEVICE_STATE_RESUMING] = 1, >>>>>>>>> + }, >>>>>>>> Our state transition diagram is pretty weak on reachable transitions >>>>>>>> out of the _STOP state, why do we select only these two as valid? >>>>>>> I have no particular opinion on specific states here, however adding >>>>>>> more states means more stuff for drivers to implement and more risk >>>>>>> driver writers will mess up this uAPI. >>>>>> _STOP == 000b => Device Stopped, not saving or resuming (from UAPI). >>>>>> >>>>>> This is the default initial state and not RUNNING. >>>>>> >>>>>> The user application should move device from STOP => RUNNING or STOP => >>>>>> RESUMING. >>>>>> >>>>>> Maybe we need to extend the comment in the UAPI file. >>>>> include/uapi/linux/vfio.h: >>>>> ... >>>>> * +------- _RESUMING >>>>> * |+------ _SAVING >>>>> * ||+----- _RUNNING >>>>> * ||| >>>>> * 000b => Device Stopped, not saving or resuming >>>>> * 001b => Device running, which is the default state >>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>> ... >>>>> * State transitions: >>>>> * >>>>> * _RESUMING _RUNNING Pre-copy Stop-and-copy _STOP >>>>> * (100b) (001b) (011b) (010b) (000b) >>>>> * 0. Running or default state >>>>> * | >>>>> ^^^^^^^^^^^^^ >>>>> ... >>>>> * 0. Default state of VFIO device is _RUNNING when the user application starts. >>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>>> >>>>> The uAPI is pretty clear here. A default state of _STOP is not >>>>> compatible with existing devices and userspace that does not support >>>>> migration. Thanks, >>>> Why do you need this state machine for userspace that doesn't support >>>> migration ? >>> For userspace that doesn't support migration, there's one state, >>> _RUNNING. That's what we're trying to be compatible and consistent >>> with. Migration is an extension, not a base requirement. >> Userspace without migration doesn't care about this state. >> >> We left with kernel now. vfio-pci today doesn't support migration, right >> ? state is in theory is 0 (STOP). >> >> This state machine is controlled by the migration SW. The drivers don't >> move state implicitly. >> >> mlx5-vfio-pci support migration and will work fine with non-migration SW >> (it will stay with state = 0 unless someone will move it. but nobody >> will) exactly like vfio-pci does today. >> >> So where is the problem ? > So you have a device that's actively modifying its internal state, > performing I/O, including DMA (thereby dirtying VM memory), all while > in the _STOP state? And you don't see this as a problem?
I don't see how is it different from vfio-pci situation.
And you said you're worried from compatibility. I can't see a compatibility issue here.
Maybe we need to rename STOP state. We can call it READY or LIVE or NON_MIGRATION_STATE.
> > There's a major inconsistency if the migration interface is telling us > something different than we can actually observe through the behavior of > the device. Thanks, > > Alex >
| |