Re: [PATCH v6 1/1] vfio/qat: Add vfio_pci driver for Intel QAT SR-IOV VF devices

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Apr 19, 2024 at 05:23:30AM +0000, Tian, Kevin wrote:
> > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Sent: Friday, April 19, 2024 6:55 AM
> > 
> > On Wed, 17 Apr 2024 22:31:41 +0800
> > Xin Zeng <xin.zeng@xxxxxxxxx> wrote:
> > 
> > > +
> > > +	/*
> > > +	 * As the device is not capable of just stopping P2P DMAs, suspend
> > the
> > > +	 * device completely once any of the P2P states are reached.
> > > +	 * On the opposite direction, resume the device after transiting from
> > > +	 * the P2P state.
> > > +	 */
> > > +	if ((cur == VFIO_DEVICE_STATE_RUNNING && new ==
> > VFIO_DEVICE_STATE_RUNNING_P2P) ||
> > > +	    (cur == VFIO_DEVICE_STATE_PRE_COPY && new ==
> > VFIO_DEVICE_STATE_PRE_COPY_P2P)) {
> > > +		ret = qat_vfmig_suspend(qat_vdev->mdev);
> > > +		if (ret)
> > > +			return ERR_PTR(ret);
> > > +		return NULL;
> > > +	}
> > 
> > This doesn't appear to be a valid way to support P2P, the P2P states
> > are defined as running states.  The guest driver may legitimately
> > access and modify the device state during P2P states. 
> 
> yes it's a conceptual violation of the definition of the P2P states.

It depends what suspend actually does.

Like if it halts all queues and keeps them halted, while still
allowing queue head/tail pointer updats then it would be a fine
implementation for P2P.

> > Should this device be advertising support for P2P?
> 
> Jason suggests all new migration drivers must support P2P state.
> In an old discussion [1]

I did? I don't think that is what the link says..

We've been saying for a while that devices should try hard to
implement P2P because if they don't then multi VFIO VMM's won't work
and people will be unhappy..

> But obviously we overlooked that by definition RUNNING_P2P is
> a running state so could still see state changed from either CPU
> or other devices.

Yes, incoming MMIO writes that change the state must be recorded, but
as long as DMA does not start and remains suspended then it is OK.

Jason




[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]
  Powered by Linux