Cherry G.Mathew wrote:
diff --git a/sys/arch/xen/xen/privcmd.c b/sys/arch/xen/xen/privcmd.c index f584913..033a37d 100644 --- a/sys/arch/xen/xen/privcmd.c +++ b/sys/arch/xen/xen/privcmd.c [...] + mfn = kmem_alloc(sizeof(u_long) * pmb->num, KM_SLEEP); Why have you removed the if (mfn == NULL) /* bailout */ check ?
Sorry, my bad. if (mfn == NULL) return ENOMEM; should be there.
+ error = copyin(pmb->arr, mfn, sizeof(u_long) * pmb->num); Good, but kmem_alloc() doesn't zero mem - I would use kmem_zalloc() since you're copyout()ing later.
Yes, it doesn't hurt to zero memory if returning it to the user. Who knows what might be there previously.
+ /* Map the memory region directly */
+ error = privcmd_map(pmap, va0, mfn, prot, pmb->num, pmb->dom);
+ if (error) {
+ kmem_free(mfn, sizeof(u_long) * pmb->num);
return error;
+ }
+ copyout(mfn, pmb->arr, sizeof(u_long) * pmb->num);
+ kmem_free(mfn, sizeof(u_long) * pmb->num);
break;
}
@@ -452,6 +426,24 @@ privcmd_ioctl(void *v)
return error;
}
+static int privcmd_map(pmap_t pmap, vaddr_t va0, u_long *mfn,
+ vm_prot_t prot, int num, int domid)
+{
+ int i;
+ vaddr_t va;
+ paddr_t ma;
+
+ for(i = 0; i< num; ++i) {
+ va = va0 + (i * PAGE_SIZE);
+ ma = ((paddr_t)mfn[i])<< PGSHIFT;
+ if (pmap_enter_ma(pmap, va, ma, 0, prot, PMAP_CANFAIL, domid))
+ mfn[i] |= 0xF0000000;
I know this is verbatim from the old code, but can you explain the
constant 0xF0000000 ? I'd prefer to have a #define or const here.
Not sure why 0xf0000000 exactly, but my guess is that the four upper bits are never used, so it is used to mark memory as not mapped (error). If it's ok I will define a MASK_INVALID with this value, does that sound better?
From libxc code I think than returning anything different from the original value marks the page as invalid (see tools/libxc/xc_foreign_memory.c:108).
As a side note, Linux uses exactly the same, but I haven't been able to find any comment regarding why 0xf0000000 is used (see drivers/xen/privcmd.c:261).
[...] Cheers,
Thanks for the review!