On Thu, Jun 17, 2021 at 12:07:10PM -0400, Dave Voutila wrote:
>
> Dave Voutila writes:
>
> > The virtio spec has had a de facto command for drivers to read the
> > serial number off a virtual block device. QEMU introduced this feature
> > years ago. Last November, the virtio governing group voted in favor of
> > adopting it officially into v1.2 (the next virtio spec) [1].
> >
> > The below diff adds the basics of handling the request returning an
> > empty serial number. (Serial numbers are limited to 20 bytes.) This
> > stops vmd from complaining about "unsupported command 0x8" when guests
> > send this command type.
>
> Got some feedback off-list from claudio@ that I think is sound. Instead
> of providing an "empty" serial id/number, simply return an UNSUPP status
> to indicate we don't support the value.
>
> I think this approach better than the approach I was suggesting that was
> based off QEMU's design of defaulting to "". (FreeBSD's Bhyve generates
> a serial like "BHYVE-1122-3344-5566" where the suffix is some truncated
> md5 of the backing filename. I'm not a fan of this approach.)
>
> >
> > secdata_desc{,idx} variables are renamed to just data_desc{,idx} to
> > semantically match the change since they're used for more than sector
> > data.
>
> I undid this renaming for now to reduce noise.
>
> > This is primarily part of my work to clean up and bring vmd's virtio
> > implementation more up to date and to align to our own
> > v{io,ioblk,ioscsi,etc.}(4) current capabilities. (vioblk(4) doesn't
> > support this yet, but Linux guests use it frequently.)
>
> While adding the BLK_ID support, I also switched the FLUSH/FLUSH_OUT
> response to be VIRTIO_BLK_S_UNSUPP as well since the device does not
> negotiate that feature. Any request from the guest to "flush" currently
> doesn't do anything (some hypervisors will fsync(2) the underlying fd)
> but for now I'm correcting the response code.
>
> I also noticed and added read/write checks prior to calls to
> {read,write}_mem. The virtio spec says a device MUST not write to a
> read-only descriptor and SHOULD NOT read a write-only descriptor with an
> exception being made for debugging. (See 2.6.5.1 Device requirements:
> The Virtqueue Descriptor Table.)
>
> Next steps in this are of code will be to properly implement the missing
> VIRTIO_BLK_S_IOERR results for failed i/o. Right now the device bails
> processing the command and doesn't reply to the driver, which is not
> conforming with virtio spec.
>
> > OK?
>
> Any other feedback? OK?
>ok mlarkin > > > > -dv > > > > [1] https://www.oasis-open.org/committees/ballot.php?id=3536 > > > Index: virtio.c > =================================================================== > RCS file: /cvs/src/usr.sbin/vmd/virtio.c,v > retrieving revision 1.89 > diff -u -p -r1.89 virtio.c > --- virtio.c 16 Jun 2021 16:55:02 -0000 1.89 > +++ virtio.c 17 Jun 2021 15:57:56 -0000 > @@ -517,6 +517,11 @@ vioblk_notifyq(struct vioblk_dev *dev) > } > > /* Read command from descriptor ring */ > + if (cmd_desc->flags & VRING_DESC_F_WRITE) { > + log_warnx("vioblk: unexpected writable cmd descriptor " > + "%d", cmd_desc_idx); > + goto out; > + } > if (read_mem(cmd_desc->addr, &cmd, sizeof(cmd))) { > log_warnx("vioblk: command read_mem error @ 0x%llx", > cmd_desc->addr); > @@ -541,6 +546,13 @@ vioblk_notifyq(struct vioblk_dev *dev) > struct ioinfo *info; > const uint8_t *secdata; > > + if ((secdata_desc->flags & VRING_DESC_F_WRITE) > + == 0) { > + log_warnx("vioblk: unwritable data " > + "descriptor %d", secdata_desc_idx); > + goto out; > + } > + > info = vioblk_start_read(dev, > cmd.sector + secbias, secdata_desc->len); > > @@ -607,6 +619,13 @@ vioblk_notifyq(struct vioblk_dev *dev) > do { > struct ioinfo *info; > > + if (secdata_desc->flags & VRING_DESC_F_WRITE) { > + log_warnx("wr vioblk: unexpected " > + "writable data descriptor %d", > + secdata_desc_idx); > + goto out; > + } > + > info = vioblk_start_write(dev, > cmd.sector + secbias, > secdata_desc->addr, secdata_desc->len); > @@ -654,7 +673,35 @@ vioblk_notifyq(struct vioblk_dev *dev) > ds_desc_idx = cmd_desc->next & VIOBLK_QUEUE_MASK; > ds_desc = &desc[ds_desc_idx]; > > - ds = VIRTIO_BLK_S_OK; > + ds = VIRTIO_BLK_S_UNSUPP; > + break; > + case VIRTIO_BLK_T_GET_ID: > + secdata_desc_idx = cmd_desc->next & VIOBLK_QUEUE_MASK; > + secdata_desc = &desc[secdata_desc_idx]; > + > + /* > + * We don't support this command yet. While it's not > + * officially part of the virtio spec (will be in v1.2) > + * there's no feature to negotiate. Linux drivers will > + * often send this command regardless. > + * > + * When the command is received, it should appear as a > + * chain of 3 descriptors, similar to the IN/OUT > + * commands. The middle descriptor should have have a > + * length of VIRTIO_BLK_ID_BYTES bytes. > + */ > + if ((secdata_desc->flags & VRING_DESC_F_NEXT) == 0) { > + log_warnx("id vioblk: unchained vioblk data " > + "descriptor received (idx %d)", > + cmd_desc_idx); > + goto out; > + } > + > + /* Skip the data descriptor. */ > + ds_desc_idx = secdata_desc->next & VIOBLK_QUEUE_MASK; > + ds_desc = &desc[ds_desc_idx]; > + > + ds = VIRTIO_BLK_S_UNSUPP; > break; > default: > log_warnx("%s: unsupported command 0x%x", __func__, > @@ -666,6 +713,11 @@ vioblk_notifyq(struct vioblk_dev *dev) > break; > } > > + if ((ds_desc->flags & VRING_DESC_F_WRITE) == 0) { > + log_warnx("%s: ds descriptor %d unwritable", __func__, > + ds_desc_idx); > + goto out; > + } > if (write_mem(ds_desc->addr, &ds, ds_desc->len)) { > log_warnx("%s: can't write device status data @ 0x%llx", > __func__, ds_desc->addr);
