Date:   Wed, 16 Feb 2022 02:52:55 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Jason Gunthorpe <jgg@...dia.com>
CC:     Yishai Hadas <yishaih@...dia.com>,
        "alex.williamson@...hat.com" <alex.williamson@...hat.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "saeedm@...dia.com" <saeedm@...dia.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "leonro@...dia.com" <leonro@...dia.com>,
        "kwankhede@...dia.com" <kwankhede@...dia.com>,
        "mgurtovoy@...dia.com" <mgurtovoy@...dia.com>,
        "maorg@...dia.com" <maorg@...dia.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "shameerali.kolothum.thodi@...wei.com" 
        <shameerali.kolothum.thodi@...wei.com>
Subject: RE: [PATCH V7 mlx5-next 09/15] vfio: Extend the device migration
 protocol with RUNNING_P2P

> From: Jason Gunthorpe <jgg@...dia.com>
> Sent: Tuesday, February 15, 2022 11:56 PM
> 
> > Defining RUNNING_P2P in above way implies that RUNNING_P2P inherits
> > all behaviors in RUNNING except blocking outbound P2P:
> > 	* generate interrupts and DMAs
> > 	* respond to MMIO
> > 	* all vfio regions are functional
> > 	* device may advance its internal state
> > 	* drain and block outstanding P2P requests
> 
> Correct.
> 
> The device must be able to recieve and process any MMIO P2P
> transaction during this state.
> 
> We discussed and left interrupts as allowed behavior.
> 
> > I think this is not the intended behavior when NDMA was being discussed
> > in previous threads, as above definition suggests the user could continue
> > to submit new requests after outstanding P2P requests are completed
> given
> > all vfio regions are functional when the device is in RUNNING_P2P.
> 
> It is the desired behavior. The device must internally stop generating
> DMA from new work, it cannot rely on external things not poking it
> with MMIO, because the whole point of the state is that MMIO P2P is
> still allowed to happen.
> 
> What gets confusing is that in normal cases I wouldn't expect any P2P
> activity to trigger a new work submission.
> 
> Probably, since many devices can't implement this, we will end up with
> devices providing a weaker version where they do RUNNING_P2P but this
> relies on the VM operating the device "sanely" without programming P2P
> work submission. It is similar to your notion that migration requires
> guest co-operation in the vPRI case.
> 
> I don't like it, and better devices really should avoid requiring
> guest co-operation, but it seems like where things are going.

Make sense to me now. 

btw can disabling PCI bus master be a general means for devices which
don't have a way of blocking P2P to implement RUNNING_P2P? 

> 
> > Though just a naming thing, possibly what we really require is a
> STOPPING_P2P
> > state which indicates the device is moving to the STOP (or STOPPED)
> > state.
> 
> No, I've deliberately avoided STOP because this isn't anything like
> STOP. It is RUNNING with one restriction.

With above explanation I'm fine with it.

> 
> > In this state the device is functional but vfio regions are not so the user still
> > needs to restrict device access:
> 
> The device is not functional in STOP. STOP means the device does not
> provide working MMIO. Ie mlx5 devices will discard all writes and
> read all 0's when in STOP.

btw I used 'STOPPING' to indicate a transitional state between RUNNING
and STOP thus its definition could be defined separately from STOP. But 
it doesn't matter now.

> 
> The point of RUNNING_P2P is to allow the device to continue to recieve
> all MMIO while halting generation of MMIO to other devices.
> 
> > In virtualization this means Qemu must stop vCPU first before entering
> > STOPPING_P2P for a device.
> 
> This is already the case. RUNNING/STOP here does not refer to the
> vCPU, it refers to this device.

I know that point. Originally I thought that having 'RUNNING' in RUNNING_P2P
implies that vCPU doesn't need to be stopped first given all vfio regions are
functional. But now I think the rationale is clear. If guest-operation exists
then vCPU can be active when entering RUNNING_P2P since the guest will
guarantee no P2P submission (via vCPU or via P2P). Otherwise vCPU must be 
stopped first to block potential P2P work submissions as a brute-force operation.

> 
> > Back to your earlier suggestion on reusing RUNNING_P2P to cover vPRI
> > usage via a new capability bit [1]:
> >
> >     "A cap like "running_p2p returns an event fd, doesn't finish until the
> >     VCPU does stuff, and stops pri as well as p2p" might be all that is
> >     required here (and not an actual new state)"
> >
> > vPRI requires a RUNNING semantics. A new capability bit can change
> > the behaviors listed above for STOPPING_P2P to below:
> > 	* both P2P and vPRI requests should be drained and blocked;
> > 	* all vfio regions are functional (with a RUNNING behavior) so
> > 	  vCPUs can continue running to help drain vPRI requests;
> > 	* an eventfd is returned for the user to poll-wait the completion
> > 	  of state transition;
> 
> vPRI draining is not STOP either. If the device is expected to provide
> working MMIO it is not STOP by definition.
> 
> > One additional requirement in driver side is to dynamically mediate the
> > fast path and queue any new request which may trigger vPRI or P2P
> > before moving out of RUNNING_P2P. If moving to STOP_COPY, then
> > queued requests will also be included as device state to be replayed
> > in the resuming path.
> 
> This could make sense. I don't know how you dynamically mediate
> though, or how you will trap ENQCMD..

Qemu can ask KVM to temporarily clear EPT mapping of the cmd portal 
to enable mediation on src and then restore the mapping before resuming
vCPU on dest. In our internal POC the cmd portal address is hard coded
in Qemu which is not good. Possibly we need a general mechanism so
migration driver which supports vPRI and extended RUNNING_P2P behavior
can report to the user a list of pages which must be accessed via read()/
write() instead of mmap when the device is in RUNNING_P2P and vCPUs
are active. Based on that information Qemu can zap related EPT mappings 
before moving the device to RUNNING_P2P.

> 
> > Does above sound a reasonable understanding of this FSM mechanism?
> 
> Other than mis-using the STOP label, it is close yes.
> 

Thanks
Kevin

Powered by blists - more mailing lists