On Mon, Jun 22, 2015 at 01:56:42PM +0000, Antti Kantee wrote:
> 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.
>
The NetBSD code use op to calculate offset inside hypercall page. I
think it is for boundary check.
I don't think this is essential though. I don't really care if a guest
issues invalid hypercall crashes or gets EINVAL.
I will remove that check.
> >+#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()
>
Done.
> >+ 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.
>
I will turn the KASSERT to "return EINVAL".
> >+ /* 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/... ?
>
DomU will crash. I will revise that comment.
> >+ 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.
>
Fixed.
> >+ 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 think so. See below.
> 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?
>
In file included from ./machine/cpufunc.h:3:0,
from
/local/scratch/Rump-kernels/rumprun/platform/xen/rumpxendev/../rump/include/x86/lock.h:68,
from
/local/scratch/Rump-kernels/rumprun/platform/xen/rumpxendev/../rump/include/machine/lock.h:3,
from
/local/scratch/Rump-kernels/rumprun/platform/xen/rumpxendev/../rump/include/sys/lock.h:74,
from
/local/scratch/Rump-kernels/rumprun/platform/xen/rumpxendev/../rump/include/uvm/uvm_extern.h:465,
from
/local/scratch/Rump-kernels/rumprun/platform/xen/rumpxendev/../rump/include/sys/vnode.h:73,
from
/local/scratch/Rump-kernels/rumprun/platform/xen/rumpxendev/privcmd.c:42:
./x86/cpufunc.h:141:28: error: macro "wrmsr" requires 3 arguments, but only 2
given
Wrmsr is defined in platform/xen/xen/include/mini-os/x86/os.h as a macro
that takes 3 arguments. I can fix that in minios header.
> >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.
>
>
I will keep that stuff there to make future update easier.
Wei.
> This patch is fine by me, use your own discretion to decide if you want to
> make the small changes or not.
