Re: [PATCH v2 4/6] vduse-blk: implements vduse-blk export

[Yongji Xie]

On Wed, Mar 16, 2022 at 8:16 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, Mar 15, 2022 at 07:52:03PM +0800, Yongji Xie wrote:
> > On Tue, Mar 15, 2022 at 7:08 PM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > >
> > > On Tue, Feb 15, 2022 at 06:59:41PM +0800, Xie Yongji wrote:
> > > > This implements a VDUSE block backends based on
> > > > the libvduse library. We can use it to export the BDSs
> > > > for both VM and container (host) usage.
> > > >
> > > > The new command-line syntax is:
> > > >
> > > > $ qemu-storage-daemon \
> > > >     --blockdev file,node-name=drive0,filename=test.img \
> > > >     --export vduse-blk,node-name=drive0,id=vduse-export0,writable=on
> > > >
> > > > After the qemu-storage-daemon started, we need to use
> > > > the "vdpa" command to attach the device to vDPA bus:
> > > >
> > > > $ vdpa dev add name vduse-export0 mgmtdev vduse
> > >
> > > The per-QEMU export id is used as the global vdpa device name. If this
> > > becomes a problem in the future then a new --export
> > > vduse-blk,vdpa-dev-name= option can be added.
> > >
> >
> > Yes.
> >
> > > > +    case VIRTIO_BLK_T_GET_ID: {
> > > > +        size_t size = MIN(iov_size(&elem->in_sg[0], in_num),
> > > > +                          VIRTIO_BLK_ID_BYTES);
> > > > +        snprintf(elem->in_sg[0].iov_base, size, "%s", 
> > > > vblk_exp->export.id);
> > >
> > > Please use iov_from_buf(). The driver is allowed to submit as many
> > > in_sg[] elements as it wants and a compliant virtio-blk device
> > > implementation must support that.
> > >
> >
> > Got it.
> >
> > > Here is the VIRTIO specification section that covers message framing:
> > > https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-280004
> > >
> > > > +    features = (1ULL << VIRTIO_F_IOMMU_PLATFORM) |
> > > > +               (1ULL << VIRTIO_F_VERSION_1) |
> > > > +               (1ULL << VIRTIO_RING_F_EVENT_IDX) |
> > > > +               (1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |
> > > > +               (1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> > > > +               (1ULL << VIRTIO_BLK_F_SIZE_MAX) |
> > > > +               (1ULL << VIRTIO_BLK_F_SEG_MAX) |
> > > > +               (1ULL << VIRTIO_BLK_F_TOPOLOGY) |
> > > > +               (1ULL << VIRTIO_BLK_F_BLK_SIZE);
> > >
> > > The VIRTIO_F_ and VIRTIO_RING_F_ feature bits report the capabilities of
> > > libvduse. They should probably be defined in libvduse. That way no
> > > changes to vduse-blk.c are required when libvduse changes:
> > >
> > >   features = LIBVDUSE_VIRTIO_FEATURES |
> > >              (1ULL << VIRTIO_BLK_F_SIZE_MAX) |
> > >              ...;
> >
> > It's OK to define the VIRTIO_F_* feature bits in libvduse since daemon
> > might not want to disable it. But maybe not including VIRTIO_RING_F_*
> > feature bits since daemon might want to disable them in some cases.
>
> The VIRTIO_RING_F_* functionality is implemented inside libvduse and not
> the device code. If the device wants to disable specific features, it
> can clear those bits.
>
> Thinking about the LIBVDUSE_VIRTIO_FEATURES idea I think it's slightly
> better to make it a function so that libvduse code supports dynamic
> linking. That way the device calls libvduse to query the feature bits
> instead of compiling a constant from the libvduse header file into the
> device executable.
>
> So something like:
>
>   features = (vdu_get_virtio_features() & ~(...features we wish to clear...)) 
> |
>              (1ULL << VIRTIO_BLK_F_SIZE_MAX) |
>              ...;
>

OK, it looks good to me.

Thanks,
Yongji