Re: [PATCH v16 Kernel 1/7] vfio: KABI for migration interface for device state

[Kirti Wankhede]

On 3/26/2020 4:11 PM, Cornelia Huck wrote:
> On Wed, 25 Mar 2020 01:02:33 +0530
> Kirti Wankhede <address@hidden> wrote:
>
> > - Defined MIGRATION region type and sub-type.
> >
> > - Defined vfio_device_migration_info structure which will be placed at the
> >    0th offset of migration region to get/set VFIO device related
> >    information. Defined members of structure and usage on read/write access.
> >
> > - Defined device states and state transition details.
> >
> > - Defined sequence to be followed while saving and resuming VFIO device.
> >
> > Signed-off-by: Kirti Wankhede <address@hidden>
> > Reviewed-by: Neo Jia <address@hidden>
> > ---
> >   include/uapi/linux/vfio.h | 228 ++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 228 insertions(+)
>
> (...)
>
> > +struct vfio_device_migration_info {
> > +       __u32 device_state;         /* VFIO device state */
> > +#define VFIO_DEVICE_STATE_STOP      (0)
> > +#define VFIO_DEVICE_STATE_RUNNING   (1 << 0)
> > +#define VFIO_DEVICE_STATE_SAVING    (1 << 1)
> > +#define VFIO_DEVICE_STATE_RESUMING  (1 << 2)
> > +#define VFIO_DEVICE_STATE_MASK      (VFIO_DEVICE_STATE_RUNNING | \
> > +                                    VFIO_DEVICE_STATE_SAVING |  \
> > +                                    VFIO_DEVICE_STATE_RESUMING)
> > +
> > +#define VFIO_DEVICE_STATE_VALID(state) \
> > +       (state & VFIO_DEVICE_STATE_RESUMING ? \
> > +       (state & VFIO_DEVICE_STATE_MASK) == VFIO_DEVICE_STATE_RESUMING : 1)
> > +
> > +#define VFIO_DEVICE_STATE_IS_ERROR(state) \
> > +       ((state & VFIO_DEVICE_STATE_MASK) == (VFIO_DEVICE_STATE_SAVING | \
> > +                                             VFIO_DEVICE_STATE_RESUMING))
> > +
> > +#define VFIO_DEVICE_STATE_SET_ERROR(state) \
> > +       ((state & ~VFIO_DEVICE_STATE_MASK) | VFIO_DEVICE_SATE_SAVING | \
> > +                                            VFIO_DEVICE_STATE_RESUMING)
> > +
> > +       __u32 reserved;
> > +       __u64 pending_bytes;
> > +       __u64 data_offset;
> > +       __u64 data_size;
> > +} __attribute__((packed));
>
> The 'packed' should not even be needed, I think?

Right, Above structure is padded properly. Removing it.

> > +
> >   /*
> >    * The MSIX mappable capability informs that MSIX data of a BAR can be mmapped
> >    * which allows direct access to non-MSIX registers which happened to be 
> > within
>
> Generally, this looks sane to me; however, we should really have
> something under Documentation/ in the long run that describes how this
> works, so that you can find out about the protocol without having to
> dig through headers.

But the documentation will have almost the same text as in this comment. 
Should we replicate it?

Thanks,
Kirti