Hi - Some (hopefully minor) comments below.
Overall, I have a couple issues with virtio that we'll need to address, though we don't need to do it now. I think I brought these up when Mike did the first round of virtio improvements. First is how we will handle multiple devices of the same type. The current MMIO registration system seems to have one device per type baked in. I think. For instance, for virtio-net, we say vm->virtio_mmio_devices[VIRTIO_MMIO_NETWORK_DEV] = &net_mmio_dev; What do we do when we have two NICs? Likewise with virtio-blk. The other big issue is with virtio-mmio and concurrency. Do we have any protections against multiple OS threads that try to setup a device at the same time? From looking at the code, I don't see it. You could get into situations where there are two ctlr threads handling virtio-mmio writes at the same time for the same device. On a related note, we shouldn't be using eventfd for synchronization. If we don't have the tools for user-level sync in the 2LS, let me know and I'll add them. Notes specific to this patch below: On 2016-07-20 at 16:52 Kyle Milka <[email protected]> wrote: > We were able to get various Linux kernels to mount a disk image. Used > linux/lguest.c and virtio_net.c as starting points. This included adding > the block device struct and request and init functions. Don't forget to run scripts/checkpatch.pl on your patches. Here's the output from this one, with my comments in (): ERROR: do not initialise globals to 0 or NULL #167: FILE: user/vmm/virtio_blk.c:12: +int debug_virtio_blk = 0; (not a huge deal. it cuts down on the size of the binary, since anything in the BSS will be zeroed automatically). ERROR: Macros with complex values should be enclosed in parentheses #169: FILE: user/vmm/virtio_blk.c:14: +#define DPRINTF(fmt, ...) \ + if (debug_virtio_blk) { \ + fprintf(stderr, "virtio_blk: " fmt, ##__VA_ARGS__); \ + } (this macro might have issues if it's used in an if-else setup) WARNING: Missing a blank line after declarations #188: FILE: user/vmm/virtio_blk.c:33: + struct stat stat_result; + if (stat(filename, &stat_result) == -1) (i'd also put the declaration at the top of the function) WARNING: quoted string split across lines #218: FILE: user/vmm/virtio_blk.c:63: + "The service function for queue '%s' was launched " + "before the driver set QueueReady to 0x1.", (the intent of this complaint is to not break a line that might be grepped for. for example, i might grep "was launched before" and not find the line. WARNING: Missing a blank line after declarations #285: FILE: user/vmm/virtio_blk.c:130: + char *pf = ""; + for (int i = 0; i < iov[olen].iov_len; i += 2) { WARNING: Missing a blank line after declarations #287: FILE: user/vmm/virtio_blk.c:132: + uint8_t *p = (uint8_t *)iov[olen].iov_base + i; + fprintf(stderr, "%s%02x", pf, *(p + 1)); total: 2 errors, 4 warnings, 247 lines checked ../patches/0001-Implemented-virtio-block.patch has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. some other comments below: > diff --git a/tests/vmm/vmrunkernel.c b/tests/vmm/vmrunkernel.c > @@ -384,6 +416,7 @@ int main(int argc, char **argv) > argc--, argv++; > // switches ... > // Sorry, I don't much like the gnu opt parsing code. > + // TODO(dcross): Convert this to use getopt() Or argp, which has built-in command documentation. See perf for an example. (No need to do this now). > diff --git a/user/vmm/virtio_blk.c b/user/vmm/virtio_blk.c > new file mode 100644 > index 0000000..0493c69 > --- /dev/null > +++ b/user/vmm/virtio_blk.c > +void blk_request(void *_vq) > +{ > + struct virtio_vq *vq = _vq; > + struct virtio_mmio_dev *dev = vq->vqdev->transport_dev; > + struct iovec *iov; > + uint32_t head; > + uint32_t olen, ilen; > + struct virtio_blk_outhdr *out; > + uint64_t offset; > + int64_t ret; > + size_t wlen; > + uint8_t *status; > + struct virtio_blk_config *cfg = vq->vqdev->cfg; > + uint64_t total_read = 0; > + > + DPRINTF("blk_request entered\n"); > + > + assert(vq != NULL); This assert may or may not fire before a page fault, since vq was dereferenced above. I'd do the assert before assigning dev and cfg. > + for (;;) { > + head = virtio_next_avail_vq_desc(vq, iov, &olen, &ilen); > + /* There are always three iovecs. > + * The first is the header. > + * The second is the actual data. > + * The third contains just the status byte. > + */ > + > + status = iov[2].iov_base; > + if (!status) > + VIRTIO_DEV_ERRX(vq->vqdev, "no room for status\n"); > + > + out = iov[0].iov_base; > + if (out->type & VIRTIO_BLK_T_FLUSH) > + VIRTIO_DEV_ERRX(vq->vqdev, "Flush not supported.\n"); > + > + offset = out->sector * 512; > + DPRINTF("offset is: %llu sector is: %lld\n", offset, > out->sector); > + if (lseek64(diskfd, offset, SEEK_SET) != offset) > + VIRTIO_DEV_ERRX(vq->vqdev, "Bad seek at sector %llu\n", > + out->sector); > + > + if (out->type & VIRTIO_BLK_T_OUT) { > + DPRINTF("blk device write\n"); > + > + if ((offset + iov[1].iov_len) > (cfg->capacity * 512)) > + VIRTIO_DEV_ERRX(vq->vqdev, "write past end of > file!\n"); > + > + ret = writev(diskfd, iov + 1, olen - 1); What's going on with this writev? The second iov is the data (a.k.a. &iov[1]). What is olen? Is it 3 here (since there are 3 iovecs?) In which case, we're actually writing the status byte too? If we're only ever sending iov[1], then "olen - 1" should be just "1". > + > + if (ret >= 0 && ret == iov[1].iov_len) > + *status = VIRTIO_BLK_S_OK; > + else > + *status = VIRTIO_BLK_S_IOERR; > + wlen = sizeof(*status); > + } else { > + DPRINTF("blk device read\n"); > + DPRINTF("olen is: %d ilen is: %d\n", olen, ilen); > + DPRINTF("iov[1] is: %d\n", iov[1].iov_len); > + ret = readv(diskfd, iov + olen, ilen - 1); Likewise on the read side. Perhaps olen is 1 and ilen is 2 here? If those statements are always true, then lets just use those values. If they are not true, then we need a little explanation for what is going on. For instance, virtio-net does something similar to this code, but it's quite clear that it is because there are multiple iovs beyond the header for the actual data. > + DPRINTF("read %d bytes, total_read is:%d\n", ret, > total_read); > + DPRINTF("blk device read done\n"); We probably don't need to keep all the DPRINTFs and other debug code around either, but we can keep them for a little while if yinz want. Barret -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
