add support for adding/removing virtio devices during runtime #241

M1cha · 2016-09-20T15:48:15Z

This change is

Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

4096 is the value of 'COMMAND_LINE_SIZE' Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

tavip

Thanks for a great patch, looks good to me, just a few coding style issues:

ret =
snprintf

is unusual for kernel code, can you please use

ret = snprintf

Also keep in mind that we don't have to split long strings, even though checkpatch.pl complains for lines over 80. We can manually override checkpatch.pl (see arch/lkl/script/checkpatch.sh) when it gets too pedantic

tavip · 2016-09-20T16:00:39Z

arch/lkl/kernel/syscalls.c

+	pdev = platform_device_alloc("virtio-mmio", PLATFORM_DEVID_AUTO);
+	if (!pdev) {
+		dev_err(&pdev->dev,
+			"%s: Unable to device alloc for virtio-mmio\n",


We are allowed to have strings go over the 80 column limit, so we can keep compact this and the above line

tavip · 2016-09-20T17:58:52Z

tools/lkl/lib/fs.c

-	sysfs_path[strlen("/sysfs/block/vd")] += disk_id;
+	if ((uint32_t) disk_id >= virtio_get_num_bootdevs())
+		ret =
+		    snprintf(sysfs_path, sizeof(sysfs_path),


We can merge ret and sprintf on the same line, even if the sysfs string goes beyond 80 chars limit.

tavip · 2016-09-20T18:20:30Z

tools/lkl/lib/fs.c

+		return (long)opendir_ret;
+
+	ret =
+	    snprintf(sysfs_path + sysfs_path_len,


Here as wel, we can merge ret and sprintf on the same line

You missed this one :)

M1cha · 2016-09-20T18:43:44Z

@tavip I updated the commit according to your comments.

tavip

Some more comments related to line splitting. Sorry to be pedantic, but it will help us when we get to try and upstream the code.

tavip · 2016-09-20T18:50:01Z

tools/lkl/lib/fs.c


-	sysfs_path[strlen("/sysfs/block/vd")] += disk_id;
+	if ((uint32_t) disk_id >= virtio_get_num_bootdevs())
+		ret = snprintf(sysfs_path, sizeof(sysfs_path), "/sysfs/devices/platform/virtio-mmio.%d.auto", disk_id - virtio_get_num_bootdevs());


Since this line is very long (>100), you can split this one after the string, like:

ret = snprintf(sysfs_path, sizeof(sysfs_path), "/sysfs/devices/platform/virtio-mmio.%d.auto",
disk_id - virtio_get_num_bootdevs());

tavip · 2016-09-20T18:50:03Z

tools/lkl/lib/fs.c

+	if ((uint32_t) disk_id >= virtio_get_num_bootdevs())
+		ret = snprintf(sysfs_path, sizeof(sysfs_path), "/sysfs/devices/platform/virtio-mmio.%d.auto", disk_id - virtio_get_num_bootdevs());
+	else
+		ret = snprintf(sysfs_path, sizeof(sysfs_path), "/sysfs/devices/virtio-mmio-cmdline/virtio-mmio.%d", disk_id);


Same here, you can split after the string.

tavip · 2016-09-20T18:50:24Z

tools/lkl/lib/fs.c

+		return (long)opendir_ret;
+
+	ret =
+	    snprintf(sysfs_path + sysfs_path_len,


You missed this one :)

tavip · 2016-09-20T18:50:37Z

tools/lkl/lib/virtio.c

-		return -LKL_ENOMEM;
+	if (!lkl_is_running()) {
+		avail = sizeof(lkl_virtio_devs) - (devs - lkl_virtio_devs);
+		num_bytes =


Here as well

tavip · 2016-09-20T18:50:50Z

tools/lkl/lib/virtio.c

+		return ret;
+
+	if (dev->virtio_mmio_id >= virtio_get_num_bootdevs())
+		ret =


tavip · 2016-09-20T18:50:58Z

tools/lkl/lib/virtio.c

+		    snprintf(devname, sizeof(devname), "virtio-mmio.%d.auto",
+			     dev->virtio_mmio_id - virtio_get_num_bootdevs());
+	else
+		ret =


Here as well

M1cha · 2016-09-20T19:14:09Z

@tavip no problem, upstreaming LKL would be really great :)

tavip

All looks good now. I'll wait a day or so before merging to give people chance for more reviews.

And we don't have to rebuild with SKIP_CHECKPATCH, I just realized that failing checkpatch does not stop the test process and both the build and the testing seems fine.

M1cha · 2016-09-20T21:11:16Z

btw 'drivers/virtio/virtio_mmio.c' throws warnings about a missing remove-function when removing a device.
This definitely looks like a upstream bug. I locally added an empty function to 'vm_dev->vdev.dev.release' to silence the warnings. I dunno if and what kind of cleanup this driver needs.

liuyuan10 · 2016-09-20T21:38:33Z

tools/lkl/lib/virtio.c

+	if (ret < 0)
+		return ret;
+
+	ret = lkl_sys_umount("/sysfs", 0);


If sysfs is already mounted before this function, it will umount it. Can you umount only when lkl_mount_fs returns 0?

liuyuan10 · 2016-09-20T21:39:03Z

tools/lkl/lib/fs.c

+
+	while ((dirent = lkl_readdir(dir))) {
+		if (startswith(dirent->d_name, prefix))
+			result = strdup(dirent->d_name);


break early?

liuyuan10 · 2016-09-20T21:48:11Z

tools/lkl/lib/virtio.c

+		devs += num_bytes;
+		dev->virtio_mmio_id = lkl_num_virtio_boot_devs++;
+	} else {
+		ret =


There are several "Must be called before calling lkl_start_kernel" in lkl.h. Can you correct them?

Can I add virtio devs after lkl is running concurrently in multiple threads? At least seems register_iomem() is not thread safe. If not, can you make a comment somewhere to make it clear?

I think it's not even thread safe before starting LKL. if that's right I think the comments should be updated in a different commit.

Initially I thought this PR applies to all virtio devs but seems only disk is tested. It's fine then.

thehajime · 2016-09-21T06:04:15Z

it is more like a general question but is it possible to adapt this patch to a netdev (dynamically added new virtio-net dev by a program) ? we may need to update tools/lkl/lib/net.c or else though.

Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

M1cha added 4 commits September 20, 2016 17:45

lkl: add 'lkl_is_running'

56b06a9

Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

lkl: increase 'lkl_virtio_devs' size to 4096

bf68531

4096 is the value of 'COMMAND_LINE_SIZE' Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

lkl: add support for mmio devices with large minor numbers

3681319

Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

lkl: move '__NR_create_syscall_thread' to public unistd header

a3dcc1f

Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

tavip requested changes Sep 20, 2016

View reviewed changes

tavip reviewed Sep 20, 2016

View reviewed changes

tavip approved these changes Sep 20, 2016

View reviewed changes

liuyuan10 requested changes Sep 20, 2016

View reviewed changes

lkl: add support for adding/removing virtio devices during runtime

46dc6da

Signed-off-by: Michael Zimmermann <sigmaepsilon92@gmail.com>

liuyuan10 approved these changes Sep 21, 2016

View reviewed changes

tavip merged commit e8301b1 into lkl:master Sep 21, 2016

M1cha mentioned this pull request Sep 21, 2016

registering block devices after starting the kernel #235

Closed

liuyuan10 mentioned this pull request Sep 24, 2016

Error msg: Device 'virtio0' does not have a release() function #245

Closed

add support for adding/removing virtio devices during runtime #241

add support for adding/removing virtio devices during runtime #241

Uh oh!

Conversation

M1cha commented Sep 20, 2016 • edited by thehajime Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tavip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tavip Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

M1cha commented Sep 20, 2016

Uh oh!

tavip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tavip Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

M1cha commented Sep 20, 2016

Uh oh!

tavip left a comment

Choose a reason for hiding this comment

Uh oh!

M1cha commented Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liuyuan10 Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

M1cha Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thehajime commented Sep 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

M1cha commented Sep 20, 2016 •

edited by thehajime

Loading

tavip Sep 20, 2016 •

edited

Loading

tavip Sep 20, 2016 •

edited

Loading

M1cha commented Sep 20, 2016 •

edited

Loading

liuyuan10 Sep 20, 2016 •

edited

Loading

M1cha Sep 21, 2016 •

edited

Loading