Re: Xen 3.3: Problem HVM guest

[Christoph Egger <Christoph_Egger%gmx.de@localhost>]

Manuel Bouyer wrote:
> On Tue, Aug 19, 2008 at 12:11:21AM +0200, Christoph Egger wrote:
> > Manuel Bouyer wrote:
> > > hello,
> > > sorry for the delay, I was offline for the last 3 days
> > >
> > > On Thu, Aug 14, 2008 at 11:39:23PM +0200, Christoph Egger wrote:
> > > > I found the bug:
> > > >
> > > > > > > > > - instrument privpgop_fault() to see if it gets called at all for
> > > > > > > > >  this mapping, and if it's doing the right thing.
> > > > > > > > >  There should be only one page in this object, and the machine
> > > > > > > > >  address should be 0 (pobj->maddr[maddr_i])
> > > > > > > > Yes, privpgop_fault() is called. It looks like it's called in a
> > > > > > > > loop. npages = 1 and machine address is 0.
> > > > > > > OK, it has the right data. I guess it's called in a loop because
> > > > > > > writing at the page keeps failing.
> > > > Writing at the page keeps failing because privpgop_fault()
> > > > does not handle this case:
> > > >
> > > >         if (pobj->maddr[maddr_i] == 0)
> > > >              continue; /* this has already been flagged as error */
> > > >
> > > > Removing this makes privpgop_fault() calling pmap_enter_ma()
> > > > and that makes the write access finally succeed and the HVM guest
> > > > starts.
> > > >
> > > > May I commit this change?
> > > There's a problem with this: IOCTL_PRIVCMD_MMAPBATCH uses 0 as an invalid
> > > address, so this test is useful. The test should be restored, but
> > > with a different magic value. As this is a physical address and
> > > it should be page-aligned, maybe we could use 0xfff as the magic value ?
> > Oh, I see. The page offsets are shifted away before the hypercall, so
> > we can assume page alignment.
> > 0xfff sounds reasonable, but we should use a #define instead of 
> > hardcoding it. That way, the places where the magic value is used are
> > easier to find. If a future Xen version changes its semantic again,
> > the magic value is easier to adjust.
>
> Sure.

Is attached patch fine with you?

Christoph


Index: privcmd.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/xen/privcmd.c,v
retrieving revision 1.26
diff -u -p -r1.26 privcmd.c
--- privcmd.c   16 Aug 2008 08:02:20 -0000      1.26
+++ privcmd.c   18 Aug 2008 22:24:22 -0000
@@ -56,6 +56,12 @@ __KERNEL_RCSID(0, "$NetBSD: privcmd.c,v 
 
 #define        PRIVCMD_MODE    (S_IRUSR)
 
+/* Magic value is used to mark invalid pages.
+ * This must be a value within the page-offset.
+ * Page-aligned values including 0x0 are used by the guest.
+ */ 
+#define INVALID_PAGE   0xfff
+
 struct privcmd_object {
        struct uvm_object uobj;
        paddr_t *maddr; /* array of machine address to map */
@@ -251,7 +257,7 @@ privcmd_ioctl(void *v)
                            pmb->dom)) {
                                mfn |= 0xF0000000;
                                copyout(&mfn, &pmb->arr[i], sizeof(mfn));
-                               maddr[i] = 0;
+                               maddr[i] = INVALID_PAGE;
                        } else {
                                pmap_remove(pmap_kernel(), trymap,
                                    trymap + PAGE_SIZE);
@@ -335,10 +341,8 @@ privpgop_fault(struct uvm_faultinfo *ufi
                        continue;
                if (pps[i] == PGO_DONTCARE)
                        continue;
-#ifndef XEN3
-               if (pobj->maddr[maddr_i] == 0)
+               if (pobj->maddr[maddr_i] == INVALID_PAGE)
                        continue; /* this has already been flagged as error */
-#endif
                error = pmap_enter_ma(ufi->orig_map->pmap, vaddr,
                    pobj->maddr[maddr_i], 0, ufi->entry->protection,
                    PMAP_CANFAIL | ufi->entry->protection,