Re: [PATCH] xen: introduce privcmd device

  • From: Antti Kantee <pooka@xxxxxx>
  • To: liuw@xxxxxxxxx, rumpkernel-users@xxxxxxxxxxxxx
  • Date: Mon, 22 Jun 2015 13:56:42 +0000

On 22/06/15 13:16, Wei Liu wrote:

+#pragma GCC diagnostic ignored "-Wcast-qual"
+#include <mini-os/mm.h>
+#pragma GCC diagnostic error "-Wcast-qual"

I think I just fixed that, you can remove the pragma.

+static int
+xenprivcmd_ioctl(void *v)
+{
...
+ switch (cmd) {
+ case IOCTL_PRIVCMD_HYPERCALL:
+ {
+ privcmd_hypercall_t *hc = (privcmd_hypercall_t *)ap->a_data;
+
+ if (hc->op >= (PAGE_SIZE >> 5))
+ return EINVAL;

Why does that depend on PAGE_SIZE>>5? Seems like [at least] this code will work differently if we change PAGE_SIZE to e.g. 2MB pages on amd64, and I'm not sure why.

+#if defined(__i386__)
+# define L1_PROT (0x001ULL /*_PAGE_PRESENT*/ | \
+ 0x002ULL /*_PAGE_RW*/ | \
+ 0x020ULL /*_PAGE_ACCESSED*/)
+#elif defined(__x86_64__)
+# define L1_PROT (0x001ULL /*_PAGE_PRESENT*/ | \
+ 0x002ULL /*_PAGE_RW*/ | \
+ 0x020ULL /*_PAGE_ACCESSED*/| \
+ 0x004ULL /*_PAGE_USER*/)
+#endif

Can you wrap this somehow, either by a special-purpose minios_map_frames() or minios_get_l1prot()

+ case IOCTL_PRIVCMD_MMAP:
+ {
+ int i;
+ privcmd_mmap_t *mcmd = ap->a_data;
+ privcmd_mmap_entry_t mentry;
+
+ for (i = 0; i < mcmd->num; i++) {
+ err = copyin(&mcmd->entry[i], &mentry, sizeof(mentry));
+ if (err)
+ return err;
+
+ if (mentry.npages == 0)
+ return EINVAL;
+
+ KASSERT(!(mentry.va & PAGE_MASK));

Why both return EINVAL and KASSERT() on seemingly similar errors?

I don't like interfaces which can half-succeed, but I guess you don't like them either and a proper error-case rollback is not worth it here. However, could add some comment.

+ /* Call with err argument == NULL will just abort the
+ * whole program if failed
+ */

Does "err == NULL" mean the penultimate argument and "abort the whole program if failed" means the whole domu will panic/halt/desolate/... ?

+ minios_map_frames(mentry.va, &mentry.mfn, mentry.npages,
+ 0, 0, mcmd->dom, NULL, L1_PROT);
+ }
+
+ err = 0;
+ break;
+ }
+ case IOCTL_PRIVCMD_MMAPBATCH:
+ {
+ privcmd_mmapbatch_t *pmb = ap->a_data;
+
+ if (pmb->num == 0)
+ return EINVAL;
+ KASSERT(!(pmb->addr & PAGE_MASK));

Ditto here for return vs. KASSERT.

+ KERNFS_ALLOCENTRY(dkt, M_TEMP, M_WAITOK);
+ /* XXX: Take VREG value from sys/vnode.h. Including vnode.h
+ * opens a can of worms. Remember to update that value if
+ * vnode.h changes.
+ */

What type of can of worms? Is it due to including minios headers here directly, or something else?

I don't think the value of VREG will change, the real problem is that <sys/vnode.h> may become included by something else due to upstream header changes, and the can of worms will open whether you want it or not. Can the problem be actually fixed easily?

diff --git a/platform/xen/rumpxendev/xenio.h b/platform/xen/rumpxendev/xenio.h
new file mode 100644
index 0000000..6b25733
--- /dev/null
+++ b/platform/xen/rumpxendev/xenio.h
...
+#if defined(_KERNEL)
+/* compat */
+#define IOCTL_PRIVCMD_INITDOMAIN_EVTCHN_OLD \
+ _IO('P', 1)

I guess the compat stuff is not needed since nobody can have binaries compiled against the old interface.

If you want to include it so that it's easy to update to a future version of xenio.h, ok, but if not, maybe just take the compat stuff out.


This patch is fine by me, use your own discretion to decide if you want to make the small changes or not.

Other related posts:

  1. Home
  2. rumpkernel-users
  3. Archive
  4. 06-2015