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
