On Wed, 20 Feb 2019 02:36:36 -0500 Zhao Yan <yan.y.zhao@xxxxxxxxx> wrote: > On Tue, Feb 19, 2019 at 02:09:18PM +0100, Cornelia Huck wrote: > > On Tue, 19 Feb 2019 16:52:14 +0800 > > Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote: (...) > > > + * Size of device config data is smaller than or equal to that of > > > + * device config region. > > > > Not sure if I understand that sentence correctly... but what if a > > device has more config state than fits into this region? Is that > > supposed to be covered by the device memory region? Or is this assumed > > to be something so exotic that we don't need to plan for it? > > > Device config data and device config region are all provided by vendor > driver, so vendor driver is always able to create a large enough device > config region to hold device config data. > So, if a device has data that are better to be saved after device stop and > saved/loaded in strict order, the data needs to be in device config region. > This kind of data is supposed to be small. > If the device data can be saved/loaded several times, it can also be put > into device memory region. So, it is the vendor driver's decision which device information should go via which region? With the device config data supposed to be saved/loaded in one go? (...) > > > +/* version number of the device state interface */ > > > +#define VFIO_DEVICE_STATE_INTERFACE_VERSION 1 > > > > Hm. Is this supposed to be backwards-compatible, should we need to bump > > this? > > > currently no backwords-compatible. we can discuss on that. It might be useful if we discover that we need some extensions. But I'm not sure how much work it would be. (...) > > > +/* > > > + * DEVICE STATES > > > + * > > > + * Four states are defined for a VFIO device: > > > + * RUNNING, RUNNING & LOGGING, STOP & LOGGING, STOP. > > > + * They can be set by writing to device_state field of > > > + * vfio_device_state_ctl region. > > > > Who controls this? Userspace? > > Yes. Userspace notifies vendor driver to do the state switching. Might be good to mention this (just to make it obvious). > > > + * LOGGING state is a special state that it CANNOT exist > > > + * independently. > > > > So it's not a state, but rather a modifier? > > > yes. or thinking LOGGING/not LOGGING as bit 1 of a device state, > whereas RUNNING/STOPPED is bit 0 of a device state. > They have to be got as a whole. So it is (on a bit level): RUNNING -> 00 STOPPED -> 01 LOGGING/RUNNING -> 10 LOGGING/STOPPED -> 11 > > > + * It must be set alongside with state RUNNING or STOP, i.e, > > > + * RUNNING & LOGGING, STOP & LOGGING. > > > + * It is used for dirty data logging both for device memory > > > + * and system memory. > > > + * > > > + * LOGGING only impacts device/system memory. In LOGGING state, get buffer > > > + * of device memory returns dirty pages since last call; outside LOGGING > > > + * state, get buffer of device memory returns whole snapshot of device > > > + * memory. system memory's dirty page is only available in LOGGING state. > > > + * > > > + * Device config should be always accessible and return whole config snapshot > > > + * regardless of LOGGING state. > > > + * */ > > > +#define VFIO_DEVICE_STATE_RUNNING 0 > > > +#define VFIO_DEVICE_STATE_STOP 1 > > > +#define VFIO_DEVICE_STATE_LOGGING 2 This makes it look a bit like LOGGING were an individual state, while 2 is in reality LOGGING/RUNNING... not sure how to make that more obvious. Maybe (as we are dealing with a u32): #define VFIO_DEVICE_STATE_RUNNING 0x00000000 #define VFIO_DEVICE_STATE_STOPPED 0x00000001 #define VFIO_DEVICE_STATE_LOGGING_RUNNING 0x00000002 #define VFIO_DEVICE_STATE_LOGGING_STOPPED 0x00000003 #define VFIO_DEVICE_STATE_LOGGING_MASK 0x00000002 > > > + > > > +/* action to get data from device memory or device config > > > + * the action is write to device state's control region, and data is read > > > + * from device memory region or device config region. > > > + * Each time before read device memory region or device config region, > > > + * action VFIO_DEVICE_DATA_ACTION_GET_BUFFER is required to write to action > > > + * field in control region. That is because device memory and devie config > > > + * region is mmaped into user space. vendor driver has to be notified of > > > + * the the GET_BUFFER action in advance. > > > + */ > > > +#define VFIO_DEVICE_DATA_ACTION_GET_BUFFER 1 > > > + > > > +/* action to set data to device memory or device config > > > + * the action is write to device state's control region, and data is > > > + * written to device memory region or device config region. > > > + * Each time after write to device memory region or device config region, > > > + * action VFIO_DEVICE_DATA_ACTION_GET_BUFFER is required to write to action > > > + * field in control region. That is because device memory and devie config > > > + * region is mmaped into user space. vendor driver has to be notified of > > > + * the the SET_BUFFER action after data written. > > > + */ > > > +#define VFIO_DEVICE_DATA_ACTION_SET_BUFFER 2 > > > > Let me describe this in my own words to make sure that I understand > > this correctly. > > > > - The actions are set by userspace to notify the kernel that it is > > going to get data or that it has just written data. > > - This is needed as a notification that the mmapped data should not be > > changed resp. just has changed. > we need this notification is because when userspace read the mmapped data, > it's from the ptr returned from mmap(). So, when userspace reads that ptr, > there will be no page fault or read/write system calls, so vendor driver > does not know whether read/write opertation happens or not. > Therefore, before userspace reads the ptr from mmap, it first writes action > field in control region (through write system call), and vendor driver > will not return the write system call until data prepared. > > When userspace writes to that ptr from mmap, it writes data to the data > region first, then writes the action field in control region (through write > system call) to notify vendor driver. vendor driver will return the system > call after it copies the buffer completely. > > > > So, how does the kernel know whether the read action has finished resp. > > whether the write action has started? Even if userspace reads/writes it > > as a whole. > > > kernel does not touch the data region except when in response to the > "action" write system call. Thanks for the explanation, that makes sense. (...)
