On Tue, 26 Oct 2021 12:18:51 -0300
Jason Gunthorpe [off-list ref] wrote:

On Tue, Oct 26, 2021 at 08:42:12AM -0600, Alex Williamson wrote:

quoted

quoted

This is also why I don't like it being so transparent as it is
something userspace needs to care about - especially if the HW cannot
support such a thing, if we intend to allow that.  

Userspace does need to care, but userspace's concern over this should
not be able to compromise the platform and therefore making VF
assignment more susceptible to fatal error conditions to comply with a
migration uAPI is troublesome for me.  

It is an interesting scenario.

I think it points that we are not implementing this fully properly.

The !RUNNING state should be like your reset efforts.

All access to the MMIO memories from userspace should be revoked
during !RUNNING

All VMAs zap'd.

All IOMMU peer mappings invalidated.

The kernel should directly block userspace from causing a MMIO TLP
before the device driver goes to !RUNNING.

Then the question of what the device does at this edge is not
relevant as hostile userspace cannot trigger it.

The logical way to implement this is to key off running and
block/unblock MMIO access when !RUNNING.

To me this strongly suggests that the extra bit is the correct way
forward as the driver is much simpler to implement and understand if
RUNNING directly controls the availability of MMIO instead of having
an irregular case where !RUNNING still allows MMIO but only until a
pending_bytes read.

Given the complexity of this can we move ahead with the current
mlx5_vfio and Yishai&co can come with some followup proposal to split
the freeze/queice and block MMIO?

I know how much we want this driver in, but I'm surprised that you're
advocating to cut-and-run with an implementation where we've identified
a potentially significant gap with some hand waving that we'll resolve
it later.

Deciding at some point in the future to forcefully block device MMIO
access from userspace when the device stops running is clearly a user
visible change and therefore subject to the don't-break-userspace
clause.  It also seems to presume that the device relies on the
vfio-core to block device access, whereas device implementations may
not require such if they're able to snapshot device state.  That might
also indicate that "freeze" is only an implementation specific
requirement.  Thanks,

Alex