Re: [RFC PATCH] vfio: Update/Clarify migration uAPI, add NDMA state

From: Jason Gunthorpe <jgg@nvidia.com>
To: "Tian, Kevin" <kevin.tian@intel.com>
Cc: Alex Williamson <alex.williamson@redhat.com>,
	"cohuck@redhat.com" <cohuck@redhat.com>,
	"corbet@lwn.net" <corbet@lwn.net>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
	"farman@linux.ibm.com" <farman@linux.ibm.com>,
	"mjrosato@linux.ibm.com" <mjrosato@linux.ibm.com>,
	"pasic@linux.ibm.com" <pasic@linux.ibm.com>
Subject: Re: [RFC PATCH] vfio: Update/Clarify migration uAPI, add NDMA state
Date: Mon, 10 Jan 2022 14:11:40 -0400	[thread overview]
Message-ID: <20220110181140.GH2328285@nvidia.com> (raw)
In-Reply-To: <BN9PR11MB52767CB9E4C30143065549C98C509@BN9PR11MB5276.namprd11.prod.outlook.com>

On Mon, Jan 10, 2022 at 07:55:16AM +0000, Tian, Kevin wrote:

> > > {SAVING} -> {RESUMING}
> > > 	If not supported, user can achieve this via:
> > > 		{SAVING}->{RUNNING}->{RESUMING}
> > > 		{SAVING}-RESET->{RUNNING}->{RESUMING}
> > 
> > This can be:
> > 
> > SAVING -> STOP -> RESUMING
> 
> From Alex's original description the default device state is RUNNING.
> This supposed to be the initial state on the dest machine for the
> device assigned to Qemu before Qemu resumes the device state.
> Then how do we eliminate the RUNNING state in above flow? Who
> makes STOP as the initial state on the dest node?

All of this notation should be read with the idea that the
device_state is already somehow moved away from RESET. Ie the above
notation is about what is possible once qemu has already moved the
device to SAVING.

> > This is currently buggy as-is because they cannot DMA map these
> > things, touch them with the CPU and kmap, or do, really, anything with
> > them.
> 
> Can you elaborate why mdev cannot access p2p pages?

It is just a failure of APIs in the kernel. A p2p page has no 'struct
page' so it cannot be used in a scatter list, and thus cannot be used in
dma_map_sg.

It also cannot be kmap'd, or memcpy'd from.

So, essentially, everything that a current mdev drivers try to do will
crash with a non-struct page pfn.

In principle this could all be made to work someday, but it doesn't
work now.

What I want to do is make these APIs correctly use struct page and
block all non-struct page memory from getting into them.

> > Since it doesn't bring any value to userspace, I prefer we not define
> > things in this complicated way.
> 
> So ERROR is really about unrecoverable failures. If recoverable suppose
> errno should have been returned and the device is still in the original
> state. Is this understanding correct?

Yes

> btw which errno indicates to the user that the device is back to the
> original state or in the ERROR state? or want the user to always check
> the device state upon any transition error?

IMHO it is a failing of the API that this cannot be reported back. The
fact that the system call became on-directional is a side effect of
abusing the migration region like this.

Jason