Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity

From: Max Gurtovoy <mgurtovoy@nvidia.com>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	Leon Romanovsky <leon@kernel.org>,
	Doug Ledford <dledford@redhat.com>,
	Yishai Hadas <yishaih@nvidia.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Kirti Wankhede <kwankhede@nvidia.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-pci@vger.kernel.org>,
	<linux-rdma@vger.kernel.org>, <netdev@vger.kernel.org>,
	Saeed Mahameed <saeedm@nvidia.com>,
	Cornelia Huck <cohuck@redhat.com>
Subject: Re: [PATCH mlx5-next 2/7] vfio: Add an API to check migration state transition validity
Date: Thu, 30 Sep 2021 18:32:07 +0300	[thread overview]
Message-ID: <d5b68bb7-d4d3-e9d8-1834-dba505bb8595@nvidia.com> (raw)
In-Reply-To: <20210930144752.GA67618@ziepe.ca>

On 9/30/2021 5:47 PM, Jason Gunthorpe wrote:
> On Thu, Sep 30, 2021 at 12:34:19PM +0300, Max Gurtovoy wrote:
>
>>> When we add the migration extension this cannot change, so after
>>> open_device() the device should be operational.
>> if it's waiting for incoming migration blob, it is not running.
> It cannot be waiting for a migration blob after open_device, that is
> not backwards compatible.
>
> Just prior to open device the vfio pci layer will generate a FLR to
> the function so we expect that post open_device has a fresh from reset
> fully running device state.

running also mean that the device doesn't have a clue on its internal 
state ? or running means unfreezed and unquiesced ?

>
>>> The reported state in the migration region should accurately reflect
>>> what the device is currently doing. If the device is operational then
>>> it must report running, not stopped.
>> STOP in migration meaning.
> As Alex and I have said several times STOP means the internal state is
> not allowed to change.
>
>>> driver will see RESUMING toggle off so it will trigger a
>>> de-serialization
>> You mean stop serialization ?
> No, I mean it will take all the migration data that has been uploaded
> through the migration region and de-serialize it into active device
> state.

you should feed the device way before that.

>
>>> driver will see SAVING toggled on so it will serialize the new state
>>> (either the pre-copy state or the post-copy state dpending on the
>>> running bit)
>> lets leave the bits and how you implement the state numbering aside.
> You've missed the point. This isn't a FSM. It is a series of three
> control bits that we have assigned logical meaning their combinatoins.
>
> The algorithm I gave is a control centric algorithm not a state
> centric algorithm and matches the direction Alex thought this was
> being designed for.
>   
>> If you finish resuming you can move to a new state (that we should add) =>
>> RESUMED.
> It is not a state machine. Once you stop prentending this is
> implementing a FSM Alex's position makes perfect sense.

You can look on it anyway you want. Three control bits or FSM. And I can 
look on it anyway I want.

The point is what bits/state you set during the resume phase:

1. you initialize at  _RUNNING bit == 001b. No problem.

2. state stream arrives, migration SW raise _RESUMING bit. should it be 
101b or 100b ? for now it's 100b. But according to your statement is 
should be 101b (invalid today) since device state can change. right ?

3. Then you should indicate that all the state was serialized to the 
device (actually to all the pci devices). 100b mean RESUMING and not 
RUNNING so maybe this can say RESUMED and state can't change now ?

4. all devices move to running 001b only after all devices moved to 100b.

Otherwise, devices will start changing each other internal states.

-Max.

>
> Jason