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?
>
> -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);
