pmap_remap_pages and protection (save/restore, part 1)

[Jed Davis <jdev%panix.com@localhost>]

I notice that pmap_remap_pages unconditionally sets PG_RW and PG_M on
the new PTE it creates for mapping in other domains' pages.  There
looks to have been an attempt to copy these bits from the existing
PTE, but that's #if 0'ed because it causes problems (the exact nature
of which is hidden by xend, but the effect is that domain creation
doesn't work), perhaps because in the case of IOCTL_PRIVCMD_MMAP the
page usually hasn't been faulted in before this happens.  (My
understanding of the i386 pmap module is not yet what it could be.)

But there are certain things that Xen does not want mapped writably,
even by a dom0, and which are involved in the domain save/restore
process.  If the dom0 tries to set such a PTE, the MMU update will
fail (and, without that patch I posted the other day to fix the PTP's
wire_count, this would panic the dom0 kernel).  That aside, it might
be nice for safety and general correctness if we don't give the user
process permissions it hasn't asked for.

I notice that some of the relevant parts of pmap_remap_pages appear to
have been copied from pmap_enter, which has a "prot" argument as well
as the "flags" one.  Is there any reason why it shouldn't have one?

For the immediate problem, though, I have a simpler patch, that
silently retries the operation without PG_RW (and PG_M) if it fails
with them; this also includes the above-mentioned wire_count fix:

Index: sys/arch/xen/i386/pmap.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/i386/pmap.c,v
retrieving revision 1.8.2.3
diff -u -p -r1.8.2.3 pmap.c
--- sys/arch/xen/i386/pmap.c    6 Jun 2005 12:16:10 -0000       1.8.2.3
+++ sys/arch/xen/i386/pmap.c    8 Sep 2005 01:04:40 -0000
@@ -4135,7 +4135,9 @@ pmap_remap_pages(pmap, va, pa, npages, f
        struct vm_page_md *mdpg;
        struct pv_head *old_pvh;
        struct pv_entry *pve = NULL; /* XXX gcc */
-       int error;
+       int error, dptpwire;
+
+       dptpwire = 0;
 
        XENPRINTK(("pmap_remap_pages(%p, %p, %p, %d, %08x, %d)\n",
            pmap, (void *)va, (void *)pa, npages, flags, dom));
@@ -4301,14 +4305,37 @@ pmap_remap_pages(pmap, va, pa, npages, f
        } else {
                pmap->pm_stats.resident_count++;
                pmap->pm_stats.wired_count++;
-               if (ptp)
+               if (ptp) {
+                       dptpwire = 1;
                        ptp->wire_count++;
+               }
        }
 
        //printf("pmap initial setup");
        maptp = (pt_entry_t *)vtomach((vaddr_t)&ptes[x86_btop(va)]);
+ change_pte:
        error = xpq_update_foreign(maptp, npte, dom);
-
+       if (error) {
+               if (npte & PG_RW) {
+                       /* 
+                        * XXXjld%panix.com@localhost: Some things cannot be
+                        * mapped read/write, even by a privileged
+                        * domain.  This wouldn't be necessary if
+                        * pmap_remap_pages had a "prot" parameter
+                        * like pmap_enter does.
+                        */
+                       npte &= ~(PG_RW | PG_M);
+                       //printf("pmap_remap_pages: losing write bit 
npte=%lx\n", (unsigned long)npte);
+                       goto change_pte;
+               }
+               printf("pmap_remap_pages: xpq_update_foreign failed va=%lx"
+                   " npte=%lx error=%d dptpwire=%d\n",
+                   va, (unsigned long)npte, error, dptpwire);
+               ptp->wire_count -= dptpwire;
+               /* XXX fix other stats? */
+               goto out;
+       }
+       
 shootdown_test:
        /* Update page attributes if needed */
        if ((opte & (PG_V | PG_U)) == (PG_V | PG_U)) {

-- 
(let ((C call-with-current-continuation)) (apply (lambda (x y) (x y)) (map
((lambda (r) ((C C) (lambda (s) (r (lambda l (apply (s s) l))))))  (lambda
(f) (lambda (l) (if (null? l) C (lambda (k) (display (car l)) ((f (cdr l))
(C k)))))))    '((#\J #\d #\D #\v #\s) (#\e #\space #\a #\i #\newline)))))