RE: [RFC v4 20/21] vfio-user: migration support

From: Thanos Makatos <thanos.makatos@nutanix.com>
To: John Johnson <john.g.johnson@oracle.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: RE: [RFC v4 20/21] vfio-user: migration support
Date: Tue, 15 Feb 2022 14:53:42 +0000	[thread overview]
Message-ID: <DM8PR02MB80052C4BED8059717F89F14D8B349@DM8PR02MB8005.namprd02.prod.outlook.com> (raw)
In-Reply-To: <A837B04D-911A-4E62-86FF-760DBCE4462B@oracle.com>

> -----Original Message-----
> From: John Johnson <john.g.johnson@oracle.com>
> Sent: 14 February 2022 18:50
> To: Thanos Makatos <thanos.makatos@nutanix.com>
> Cc: qemu-devel@nongnu.org
> Subject: Re: [RFC v4 20/21] vfio-user: migration support
> 
> 
> 
> > On Feb 11, 2022, at 5:31 AM, Thanos Makatos
> <thanos.makatos@nutanix.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: Qemu-devel <qemu-devel-
> >> bounces+thanos.makatos=nutanix.com@nongnu.org> On Behalf Of John
> >> Johnson
> >> Sent: 12 January 2022 00:44
> >> To: qemu-devel@nongnu.org
> >> Subject: [RFC v4 20/21] vfio-user: migration support
> >>
> >>
> >> +static int vfio_user_dirty_bitmap(VFIOProxy *proxy,
> >> +                                  struct vfio_iommu_type1_dirty_bitmap *cmd,
> >> +                                  struct vfio_iommu_type1_dirty_bitmap_get
> >> +                                  *dbitmap)
> >> +{
> >> +    g_autofree struct {
> >> +        VFIOUserDirtyPages msg;
> >> +        VFIOUserBitmapRange range;
> >> +    } *msgp = NULL;
> >> +    int msize, rsize;
> >> +
> >> +    /*
> >> +     * If just the command is sent, the returned bitmap isn't needed.
> >> +     * The bitmap structs are different from the ioctl() versions,
> >> +     * ioctl() returns the bitmap in a local VA
> >> +     */
> >> +    if (dbitmap != NULL) {
> >> +        msize = sizeof(*msgp);
> >> +        rsize = msize + dbitmap->bitmap.size;
> >> +        msgp = g_malloc0(rsize);
> >> +        msgp->range.iova = dbitmap->iova;
> >> +        msgp->range.size = dbitmap->size;
> >> +        msgp->range.bitmap.pgsize = dbitmap->bitmap.pgsize;
> >> +        msgp->range.bitmap.size = dbitmap->bitmap.size;
> >> +    } else {
> >> +        msize = rsize = sizeof(VFIOUserDirtyPages);
> >> +        msgp = g_malloc0(rsize);
> >> +    }
> >> +
> >> +    vfio_user_request_msg(&msgp->msg.hdr, VFIO_USER_DIRTY_PAGES,
> msize,
> >> 0);
> >> +    msgp->msg.argsz = rsize - sizeof(VFIOUserHdr);
> >> +    msgp->msg.flags = cmd->flags;
> >> +
> >> +    vfio_user_send_wait(proxy, &msgp->msg.hdr, NULL, rsize, false);
> >> +    if (msgp->msg.hdr.flags & VFIO_USER_ERROR) {
> >> +        return -msgp->msg.hdr.error_reply;
> >> +    }
> >
> > We need to check argsz in the response, in which case the client needs to retry
> with a larger argsz.
> >
> 
> 	The client needs to retry if the server reply can be larger than the client
> expects, such as GET_REGION_INFO, where the client doesn’t know how many
> capabilities
> will be returned a priori.
> 
> 	In this case, argsz is derived from dbitmap->bitmap.size, which was set
> in
> vfio_get_dirty_bitmap() to be large enough to cover the entire range:
> 
> pages = REAL_HOST_PAGE_ALIGN(range->size) / qemu_real_host_page_size;
> range->bitmap.size = ROUND_UP(pages, sizeof(__u64) * BITS_PER_BYTE) /
>                                          BITS_PER_BYTE;
> 
> so I’m not sure we’d ever see a case where the reply is larger than expected.

The client is doing the right thing, and most likely at this stage a different argsz would mean a server bug. I did actually run into this case because of a server bug, should we at least check and report and error if it's not what we expect?