[PATCH 0 of 4] [RFC PATCH] Various docs and inthe field help patches.

[PATCH 0 of 4] [RFC PATCH] Various docs and inthe field help patches.

[Konrad Rzeszutek Wilk]

Couple of patches that I've been having in my hg tree
that I've neglected to send out. The vga_delay I posted
some time ago and I have fixed it per review comments.

The other ones are newer and pertain to documentation and
making in the field easier to figure out what is going wrong
with a guest.

[PATCH 1 of 4] xen/vga: Add 'vga_delay' parameter to delay screen output by X miliseconds per line

[Konrad Rzeszutek Wilk]

# HG changeset patch
# User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
# Date 1345579709 14400
# Node ID 635917c6dac4ab8748572fcbeb3e745428684e15
# Parent  e6ca45ca03c2e08af3a74b404166527b68fd1218
xen/vga: Add 'vga_delay' parameter to delay screen output by X miliseconds per line.

This is useful if you find yourself on machine that has no serial console,
nor any PCI, PCIe to put in a serial card. Nothing really fancy except it allows
to capture the screenshot of the screen using a camera.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r e6ca45ca03c2 -r 635917c6dac4 docs/misc/xen-command-line.markdown
--- a/docs/misc/xen-command-line.markdown	Mon Aug 20 08:46:47 2012 +0200
+++ b/docs/misc/xen-command-line.markdown	Tue Aug 21 16:08:29 2012 -0400
@@ -606,6 +606,15 @@ The optional `keep` parameter causes Xen
 console even after dom0 has been started.  The default behaviour is to
 relinquish control to dom0.
 
+### vga_delay
+> `= <miliseconds>`
+
+> Default: `vga_delay=0`
+
+Defines the delay to print a line to the screen. '2' is a a good value
+to get a good screen output. Note: If you need to use this, do so with care
+as it will screw up time handling.
+
 ### vpid (Intel)
 > `= <boolean>`
 
diff -r e6ca45ca03c2 -r 635917c6dac4 xen/drivers/video/vga.c
--- a/xen/drivers/video/vga.c	Mon Aug 20 08:46:47 2012 +0200
+++ b/xen/drivers/video/vga.c	Tue Aug 21 16:08:29 2012 -0400
@@ -10,7 +10,7 @@
 #include <xen/mm.h>
 #include <xen/vga.h>
 #include <asm/io.h>
-
+#include <xen/delay.h>
 /* Filled in by arch boot code. */
 struct xen_vga_console_info vga_console_info;
 
@@ -49,6 +49,15 @@ void (*vga_puts)(const char *) = vga_noo
 static char __initdata opt_vga[30] = "";
 string_param("vga", opt_vga);
 
+/*
+ * 'vga_delay=miliseconds' which defines to delay to print a line
+ * to the screen. 2 is a a good value to get a good screen output.
+ * NOTE: If you need to use this, do so with care as it wil screw up
+ * time handling
+ */
+static unsigned int __read_mostly vga_delay;
+integer_param("vga_delay", vga_delay);
+
 /* VGA text-mode definitions. */
 static unsigned int columns, lines;
 #define ATTRIBUTE   7
@@ -135,6 +144,7 @@ static void vga_text_puts(const char *s)
                 ypos = lines - 1;
                 memmove(video, video + 2 * columns, ypos * 2 * columns);
                 memset(video + ypos * 2 * columns, 0, 2 * xpos);
+                mdelay(vga_delay);
             }
             xpos = 0;
         }

[PATCH 2 of 4] get_page_type: Print out extra information when failing to get page_type

[Konrad Rzeszutek Wilk]

# HG changeset patch
# User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
# Date 1345579709 14400
# Node ID 8ed3eef706710c9c476a8d984bfb2861d92bedfb
# Parent  635917c6dac4ab8748572fcbeb3e745428684e15
get_page_type: Print out extra information when failing to get page_type.

When any reference to __get_page_type is called and it fails, we get:

(XEN) mm.c:2429:d0 Bad type (saw 1400000000000002 != exp 7000000000000000) for mfn 10e392 (pfn 1bf6c)

with this patch we get some extra details such as:
(XEN) debug.c:127:d0 cr3: 10d80b000, searching for 10e392
(XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PUD/L3: [258][272]
(XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PMD/L2: [258][511][511]
(XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PGD/L4: [272]
(XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PUD/L3: [511][511]

where it actually is in the pagetable of the guest. This is useful
b/c we can figure out where it is, and use that to figure out where
the OS thinks it is.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r 635917c6dac4 -r 8ed3eef70671 xen/arch/x86/debug.c
--- a/xen/arch/x86/debug.c	Tue Aug 21 16:08:29 2012 -0400
+++ b/xen/arch/x86/debug.c	Tue Aug 21 16:08:29 2012 -0400
@@ -70,8 +70,127 @@ dbg_hvm_va2mfn(dbgva_t vaddr, struct dom
     return mfn;
 }
 
+#define LEVEL_L4 4
+#define LEVEL_L3 3
+#define LEVEL_L2 2
+#define LEVEL_L1 1
+#define UNDEFINED 0
+static void dbg_print_mfn(struct domain *dp, unsigned long mfn,
+                          int l4i, int l3i, int l2i, int l1i)
+{
+	char s[32];
+	char *p;
+	static const char *const names[] = {
+		[LEVEL_L4] = "PGD/L4",
+		[LEVEL_L3] = "PUD/L3",
+		[LEVEL_L2] = "PMD/L2",
+		[LEVEL_L1] = "PTE/L1",
+		[UNDEFINED] = "unknown",
+	};
+	unsigned level = 0;
+	p = s;
+	if (l4i >= 0) {
+		p += snprintf(p, ARRAY_SIZE(s), "[%d]", l4i);
+		level = LEVEL_L4;
+	}
+	if (l3i >= 0) {
+		p += snprintf(p, ARRAY_SIZE(s) - (p - s), "[%d]", l3i);
+		level = LEVEL_L3;
+	}
+	if (l2i >= 0) {
+		p += snprintf(p, ARRAY_SIZE(s) - (p - s), "[%d]", l2i);
+		level = LEVEL_L2;
+	}
+	if (l1i >= 0) {
+		p += snprintf(p, ARRAY_SIZE(s) - (p - s), "[%d]", l1i);
+		level = LEVEL_L1;
+	}
+	gdprintk(XENLOG_WARNING , "cr3(%lx) has mfn(%lx) in level %s: %s\n",
+			dp->vcpu[0]->arch.cr3, mfn, names[level], s);
+#undef LEVEL_L4
+#undef LEVEL_L3
+#undef LEVEL_L2
+#undef LEVEL_L1
+#undef UNDEFINED
+}
+void
+dbg_pv_mfn(unsigned long find_mfn, struct domain *dp)
+{
 #if defined(__x86_64__)
+    l4_pgentry_t l4e, *l4t;
+#endif
+    l3_pgentry_t l3e, *l3t;
+    l2_pgentry_t l2e, *l2t;
+    l1_pgentry_t l1e, *l1t;
+    unsigned long cr3 = dp->vcpu[0]->arch.cr3;
+    int l4i, l3i, l2i, l1i;
+    unsigned long mfn;
 
+	gdprintk(XENLOG_WARNING , "cr3: %lx, searching for %lx\n",
+			dp->vcpu[0]->arch.cr3, find_mfn);
+
+	l4i = l3i = l2i = l1i = 0;
+#if defined(__x86_64__)
+	for ( l4i = 0; l4i < L4_PAGETABLE_ENTRIES; l4i++ )
+	{
+
+        l4t = map_domain_page(cr3 >> PAGE_SHIFT);
+        l4e = l4t[l4i];
+        mfn = l4e_get_pfn(l4e);
+        if (mfn == find_mfn)
+            dbg_print_mfn(dp, mfn, l4i, /* L3 */-1, /* L2 */-1, /* L1 */-1);
+
+        if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
+            continue;
+        mfn = l4e_get_pfn(l4e);
+        unmap_domain_page(l4t);
+        l3t = map_domain_page(mfn);
+#else
+		/* 32-bit start */
+        l3t = map_domain_page(cr3 >> PAGE_SHIFT);
+        l3t += (cr3 & 0xFE0UL) >> 3;
+#endif
+        for ( l3i = 0; l3i < L3_PAGETABLE_ENTRIES; l3i++ )
+        {
+            l3e = l3t[l3i];
+            mfn = l3e_get_pfn(l3e);
+            if ( mfn == find_mfn )
+                dbg_print_mfn(dp, mfn, l4i, l3i, /* L2 */-1, /* L1 */-1);
+
+            if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) )
+                continue;
+            l2t = map_domain_page(mfn);
+            for ( l2i = 0; l2i < L2_PAGETABLE_ENTRIES; l2i++ )
+            {
+                l2e = l2t[l2i];
+                mfn = l2e_get_pfn(l2e);
+                if (mfn == find_mfn )
+                    dbg_print_mfn(dp, mfn, l4i, l3i, l2i, /* L1 */-1);
+
+                if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) ||
+                    (l2e_get_flags(l2e) & _PAGE_PSE) )
+                    continue;
+                l1t = map_domain_page(mfn);
+                for ( l1i = 0; l1i < L1_PAGETABLE_ENTRIES; l1i ++ )
+                {
+                    l1e = l1t[l1i];
+                    mfn = l1e_get_pfn(l1e);
+                    if ( !mfn_valid(mfn) )
+                        continue;
+                    if ( mfn == find_mfn )
+                        dbg_print_mfn(dp, mfn, l4i, l3i, l2i, l1i);
+                }
+                unmap_domain_page(l1t);
+            }
+            unmap_domain_page(l2t);
+        }
+        unmap_domain_page(l3t);
+#if defined(__x86_64__)
+	}
+#endif
+}
+
+#if defined(__x86_64__)
 /* 
  * pgd3val: this is the value of init_mm.pgd[3] in a PV guest. It is optional.
  *          This to assist debug of modules in the guest. The kernel address 
diff -r 635917c6dac4 -r 8ed3eef70671 xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c	Tue Aug 21 16:08:29 2012 -0400
+++ b/xen/arch/x86/mm.c	Tue Aug 21 16:08:29 2012 -0400
@@ -2422,6 +2422,8 @@ static int __put_page_type(struct page_i
 }
 
 
+extern void dbg_pv_mfn(unsigned long mfn, struct domain *d);
+
 static int __get_page_type(struct page_info *page, unsigned long type,
                            int preemptible)
 {
@@ -2503,6 +2505,7 @@ static int __get_page_type(struct page_i
                     "for mfn %lx (pfn %lx)",
                     x, type, page_to_mfn(page),
                     get_gpfn_from_mfn(page_to_mfn(page)));
+            dbg_pv_mfn(page_to_mfn(page), page_get_owner(page));
             return -EINVAL;
         }
         else if ( unlikely(!(x & PGT_validated)) )

Re: [PATCH 2 of 4] get_page_type: Print out extra information when failing to get page_type

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 08/21/12 10:22 PM >>>
>get_page_type: Print out extra information when failing to get page_type.
>
>When any reference to __get_page_type is called and it fails, we get:
>
>(XEN) mm.c:2429:d0 Bad type (saw 1400000000000002 != exp 7000000000000000) for mfn 10e392 (pfn 1bf6c)
>
>with this patch we get some extra details such as:
>(XEN) debug.c:127:d0 cr3: 10d80b000, searching for 10e392
>(XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PUD/L3: [258][272]
>(XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PMD/L2: [258][511][511]
>(XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PGD/L4: [272]
>(XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PUD/L3: [511][511]
>
>where it actually is in the pagetable of the guest. This is useful
>b/c we can figure out where it is, and use that to figure out where
>the OS thinks it is.

Hmm, not sure how useful this is: To me, it is first of all non-obvious why
there are two instances of L3 here, what the non-consistent indexes really
mean, and what the repeated printing of CR3 is good for.

>--- a/xen/arch/x86/debug.c    Tue Aug 21 16:08:29 2012 -0400
>+++ b/xen/arch/x86/debug.c    Tue Aug 21 16:08:29 2012 -0400

Further, putting the code in this file is likely wrong - as it says close to its
top, its a helper file for debuggers.

>--- a/xen/arch/x86/mm.c    Tue Aug 21 16:08:29 2012 -0400
>+++ b/xen/arch/x86/mm.c    Tue Aug 21 16:08:29 2012 -0400
>@@ -2422,6 +2422,8 @@ static int __put_page_type(struct page_i
>}
 >
 >
>+extern void dbg_pv_mfn(unsigned long mfn, struct domain *d);
>+
>static int __get_page_type(struct page_info *page, unsigned long type,
>int preemptible)
>{

No extern declarations in C files please.

Jan

Re: [PATCH 2 of 4] get_page_type: Print out extra information when failing to get page_type

[Jan Beulich]

>>> On 21.08.12 at 22:08, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> # HG changeset patch
> # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> # Date 1345579709 14400
> # Node ID 8ed3eef706710c9c476a8d984bfb2861d92bedfb
> # Parent  635917c6dac4ab8748572fcbeb3e745428684e15
> get_page_type: Print out extra information when failing to get page_type.
> 
> When any reference to __get_page_type is called and it fails, we get:
> 
> (XEN) mm.c:2429:d0 Bad type (saw 1400000000000002 != exp 7000000000000000) 
> for mfn 10e392 (pfn 1bf6c)
> 
> with this patch we get some extra details such as:
> (XEN) debug.c:127:d0 cr3: 10d80b000, searching for 10e392
> (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PUD/L3: [258][272]
> (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PMD/L2: [258][511][511]
> (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PGD/L4: [272]
> (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PUD/L3: [511][511]
> 
> where it actually is in the pagetable of the guest. This is useful
> b/c we can figure out where it is, and use that to figure out where
> the OS thinks it is.

In addition to my earlier reply, I also think that the printing
should be done at info level, so that nothing would get
additionally printed without special command line options.

Jan

Re: [PATCH 2 of 4] get_page_type: Print out extra information when failing to get page_type

[Konrad Rzeszutek Wilk]

On Wed, Aug 22, 2012 at 03:14:41PM +0100, Jan Beulich wrote:
> >>> On 21.08.12 at 22:08, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > # HG changeset patch
> > # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > # Date 1345579709 14400
> > # Node ID 8ed3eef706710c9c476a8d984bfb2861d92bedfb
> > # Parent  635917c6dac4ab8748572fcbeb3e745428684e15
> > get_page_type: Print out extra information when failing to get page_type.
> > 
> > When any reference to __get_page_type is called and it fails, we get:
> > 
> > (XEN) mm.c:2429:d0 Bad type (saw 1400000000000002 != exp 7000000000000000) 
> > for mfn 10e392 (pfn 1bf6c)
> > 
> > with this patch we get some extra details such as:
> > (XEN) debug.c:127:d0 cr3: 10d80b000, searching for 10e392
> > (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PUD/L3: [258][272]
> > (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PMD/L2: [258][511][511]
> > (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PGD/L4: [272]
> > (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PUD/L3: [511][511]
> > 
> > where it actually is in the pagetable of the guest. This is useful
> > b/c we can figure out where it is, and use that to figure out where
> > the OS thinks it is.
> 
> In addition to my earlier reply, I also think that the printing
> should be done at info level, so that nothing would get
> additionally printed without special command line options.

I will be more than happy to make those changes. However I think
you are feeling that this is not really that useful? Perhaps if I spiced
it up by also dumping what those 'types' are and added some
rudimentary logic troubleshooting (its an L2 type, and you are trying to
add it as an L1 entry as writeable, so on..)

> 
> Jan
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 2 of 4] get_page_type: Print out extra information when failing to get page_type

[Jan Beulich]

>>> On 22.08.12 at 16:24, Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:
> On Wed, Aug 22, 2012 at 03:14:41PM +0100, Jan Beulich wrote:
>> >>> On 21.08.12 at 22:08, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> > # HG changeset patch
>> > # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> > # Date 1345579709 14400
>> > # Node ID 8ed3eef706710c9c476a8d984bfb2861d92bedfb
>> > # Parent  635917c6dac4ab8748572fcbeb3e745428684e15
>> > get_page_type: Print out extra information when failing to get page_type.
>> > 
>> > When any reference to __get_page_type is called and it fails, we get:
>> > 
>> > (XEN) mm.c:2429:d0 Bad type (saw 1400000000000002 != exp 7000000000000000) 
>> > for mfn 10e392 (pfn 1bf6c)
>> > 
>> > with this patch we get some extra details such as:
>> > (XEN) debug.c:127:d0 cr3: 10d80b000, searching for 10e392
>> > (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PUD/L3: 
> [258][272]
>> > (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PMD/L2: 
> [258][511][511]
>> > (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PGD/L4: [272]
>> > (XEN) debug.c:111:d0 cr3(10d80b000) has mfn(10e392) in level PUD/L3: 
> [511][511]
>> > 
>> > where it actually is in the pagetable of the guest. This is useful
>> > b/c we can figure out where it is, and use that to figure out where
>> > the OS thinks it is.
>> 
>> In addition to my earlier reply, I also think that the printing
>> should be done at info level, so that nothing would get
>> additionally printed without special command line options.
> 
> I will be more than happy to make those changes. However I think
> you are feeling that this is not really that useful? Perhaps if I spiced
> it up by also dumping what those 'types' are and added some
> rudimentary logic troubleshooting (its an L2 type, and you are trying to
> add it as an L1 entry as writeable, so on..)

No, it doesn't need to go that far I think. My main concern is that
if something like that should get added, it should reduce the need
to consult the sources to understand the messages. Currently I'm
rather suspecting the inverse.

Jan

[PATCH 3 of 4] xen/pagetables: Document that all of the initial regions are mapped

[Konrad Rzeszutek Wilk]

# HG changeset patch
# User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
# Date 1345579709 14400
# Node ID 74bedb086c5b72447262e087c0218b89f8bc9140
# Parent  8ed3eef706710c9c476a8d984bfb2861d92bedfb
xen/pagetables: Document that all of the initial regions are mapped.

The documentation states that the layout of the initial region looks
as so:
   a. relocated kernel image
   b. initial ram disk              [mod_start, mod_len]
   c. list of allocated page frames [mfn_list, nr_pages]
      (unless relocated due to XEN_ELFNOTE_INIT_P2M)
   d. start_info_t structure        [register ESI (x86)]
   e. bootstrap page tables         [pt_base, CR3 (x86)]
   f. bootstrap stack               [register ESP (x86)]

But it does not clarify that the virtual address to all of
those areas is initially mapped by the pt_base (or CR3).
Lets fix that.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r 8ed3eef70671 -r 74bedb086c5b xen/include/public/xen.h
--- a/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
+++ b/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
@@ -675,6 +675,9 @@ typedef struct shared_info shared_info_t
  *  8. There is guaranteed to be at least 512kB padding after the final
  *     bootstrap element. If necessary, the bootstrap virtual region is
  *     extended by an extra 4MB to ensure this.
+ *
+ *  NOTE: The initial virtual region (3a -> 3f) are all mapped by the initial
+ *  pagetables [pt_base, CR3 (x86)].
  */
 
 #define MAX_GUEST_CMDLINE 1024

Re: [PATCH 3 of 4] xen/pagetables: Document that all of the initial regions are mapped

[Jan Beulich]

>>> On 21.08.12 at 22:08, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> # HG changeset patch
> # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> # Date 1345579709 14400
> # Node ID 74bedb086c5b72447262e087c0218b89f8bc9140
> # Parent  8ed3eef706710c9c476a8d984bfb2861d92bedfb
> xen/pagetables: Document that all of the initial regions are mapped.
> 
> The documentation states that the layout of the initial region looks
> as so:
>    a. relocated kernel image
>    b. initial ram disk              [mod_start, mod_len]
>    c. list of allocated page frames [mfn_list, nr_pages]
>       (unless relocated due to XEN_ELFNOTE_INIT_P2M)
>    d. start_info_t structure        [register ESI (x86)]
>    e. bootstrap page tables         [pt_base, CR3 (x86)]
>    f. bootstrap stack               [register ESP (x86)]
> 
> But it does not clarify that the virtual address to all of
> those areas is initially mapped by the pt_base (or CR3).
> Lets fix that.

To me this is already being said by "This the order of bootstrap
elements in the initial virtual region".

Jan

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> diff -r 8ed3eef70671 -r 74bedb086c5b xen/include/public/xen.h
> --- a/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
> +++ b/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
> @@ -675,6 +675,9 @@ typedef struct shared_info shared_info_t
>   *  8. There is guaranteed to be at least 512kB padding after the final
>   *     bootstrap element. If necessary, the bootstrap virtual region is
>   *     extended by an extra 4MB to ensure this.
> + *
> + *  NOTE: The initial virtual region (3a -> 3f) are all mapped by the initial
> + *  pagetables [pt_base, CR3 (x86)].
>   */
>  
>  #define MAX_GUEST_CMDLINE 1024
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH 3 of 4] xen/pagetables: Document that all of the initial regions are mapped

[Konrad Rzeszutek Wilk]

On Wed, Aug 22, 2012 at 03:17:04PM +0100, Jan Beulich wrote:
> >>> On 21.08.12 at 22:08, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > # HG changeset patch
> > # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > # Date 1345579709 14400
> > # Node ID 74bedb086c5b72447262e087c0218b89f8bc9140
> > # Parent  8ed3eef706710c9c476a8d984bfb2861d92bedfb
> > xen/pagetables: Document that all of the initial regions are mapped.
> > 
> > The documentation states that the layout of the initial region looks
> > as so:
> >    a. relocated kernel image
> >    b. initial ram disk              [mod_start, mod_len]
> >    c. list of allocated page frames [mfn_list, nr_pages]
> >       (unless relocated due to XEN_ELFNOTE_INIT_P2M)
> >    d. start_info_t structure        [register ESI (x86)]
> >    e. bootstrap page tables         [pt_base, CR3 (x86)]
> >    f. bootstrap stack               [register ESP (x86)]
> > 
> > But it does not clarify that the virtual address to all of
> > those areas is initially mapped by the pt_base (or CR3).
> > Lets fix that.
> 
> To me this is already being said by "This the order of bootstrap
> elements in the initial virtual region".

Stefano wanted to make sure we have it written as clear as possible.
I am going to be a good little submitter and let you guys sort this
one out  :-)

<gets some popcorn out>
> 
> Jan
> 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > diff -r 8ed3eef70671 -r 74bedb086c5b xen/include/public/xen.h
> > --- a/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
> > +++ b/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
> > @@ -675,6 +675,9 @@ typedef struct shared_info shared_info_t
> >   *  8. There is guaranteed to be at least 512kB padding after the final
> >   *     bootstrap element. If necessary, the bootstrap virtual region is
> >   *     extended by an extra 4MB to ensure this.
> > + *
> > + *  NOTE: The initial virtual region (3a -> 3f) are all mapped by the initial
> > + *  pagetables [pt_base, CR3 (x86)].
> >   */
> >  
> >  #define MAX_GUEST_CMDLINE 1024
> > 
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org 
> > http://lists.xen.org/xen-devel 
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 3 of 4] xen/pagetables: Document that all of the initial regions are mapped

[Stefano Stabellini]

On Wed, 22 Aug 2012, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 22, 2012 at 03:17:04PM +0100, Jan Beulich wrote:
> > >>> On 21.08.12 at 22:08, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > > # HG changeset patch
> > > # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > # Date 1345579709 14400
> > > # Node ID 74bedb086c5b72447262e087c0218b89f8bc9140
> > > # Parent  8ed3eef706710c9c476a8d984bfb2861d92bedfb
> > > xen/pagetables: Document that all of the initial regions are mapped.
> > > 
> > > The documentation states that the layout of the initial region looks
> > > as so:
> > >    a. relocated kernel image
> > >    b. initial ram disk              [mod_start, mod_len]
> > >    c. list of allocated page frames [mfn_list, nr_pages]
> > >       (unless relocated due to XEN_ELFNOTE_INIT_P2M)
> > >    d. start_info_t structure        [register ESI (x86)]
> > >    e. bootstrap page tables         [pt_base, CR3 (x86)]
> > >    f. bootstrap stack               [register ESP (x86)]
> > > 
> > > But it does not clarify that the virtual address to all of
> > > those areas is initially mapped by the pt_base (or CR3).
> > > Lets fix that.
> > 
> > To me this is already being said by "This the order of bootstrap
> > elements in the initial virtual region".
> 
> Stefano wanted to make sure we have it written as clear as possible.
> I am going to be a good little submitter and let you guys sort this
> one out  :-)

Let's step back for a second and see if I understand correctly: your
patch 6/11 removes the call to xen_map_identity_early on x86_64 because
"Xen provides us with all the memory mapped that we need to function".

The original xen_map_identity_early maps up to max_pfn, that is
xen_start_info->nr_pages, so I am assuming that what you meant is that
"Xen provides us with all the memory already mapped in the bootstrap
page tables".
And that is not written anywhere in the Xen headers.

Therefore, if I understand the issue correctly, I would add the
following to xen.h:

"On x86_64 the bootstrap page tables map all the pages assigned to the
domain."



> > 
> > Jan
> > 
> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > > 
> > > diff -r 8ed3eef70671 -r 74bedb086c5b xen/include/public/xen.h
> > > --- a/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
> > > +++ b/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
> > > @@ -675,6 +675,9 @@ typedef struct shared_info shared_info_t
> > >   *  8. There is guaranteed to be at least 512kB padding after the final
> > >   *     bootstrap element. If necessary, the bootstrap virtual region is
> > >   *     extended by an extra 4MB to ensure this.
> > > + *
> > > + *  NOTE: The initial virtual region (3a -> 3f) are all mapped by the initial
> > > + *  pagetables [pt_base, CR3 (x86)].
> > >   */
> > >  
> > >  #define MAX_GUEST_CMDLINE 1024

Re: [PATCH 3 of 4] xen/pagetables: Document that all of the initial regions are mapped

[Jan Beulich]

>>> On 22.08.12 at 17:24, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
wrote:
> On Wed, 22 Aug 2012, Konrad Rzeszutek Wilk wrote:
>> On Wed, Aug 22, 2012 at 03:17:04PM +0100, Jan Beulich wrote:
>> > >>> On 21.08.12 at 22:08, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>> > > # HG changeset patch
>> > > # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> > > # Date 1345579709 14400
>> > > # Node ID 74bedb086c5b72447262e087c0218b89f8bc9140
>> > > # Parent  8ed3eef706710c9c476a8d984bfb2861d92bedfb
>> > > xen/pagetables: Document that all of the initial regions are mapped.
>> > > 
>> > > The documentation states that the layout of the initial region looks
>> > > as so:
>> > >    a. relocated kernel image
>> > >    b. initial ram disk              [mod_start, mod_len]
>> > >    c. list of allocated page frames [mfn_list, nr_pages]
>> > >       (unless relocated due to XEN_ELFNOTE_INIT_P2M)
>> > >    d. start_info_t structure        [register ESI (x86)]
>> > >    e. bootstrap page tables         [pt_base, CR3 (x86)]
>> > >    f. bootstrap stack               [register ESP (x86)]
>> > > 
>> > > But it does not clarify that the virtual address to all of
>> > > those areas is initially mapped by the pt_base (or CR3).
>> > > Lets fix that.
>> > 
>> > To me this is already being said by "This the order of bootstrap
>> > elements in the initial virtual region".
>> 
>> Stefano wanted to make sure we have it written as clear as possible.
>> I am going to be a good little submitter and let you guys sort this
>> one out  :-)
> 
> Let's step back for a second and see if I understand correctly: your
> patch 6/11 removes the call to xen_map_identity_early on x86_64 because
> "Xen provides us with all the memory mapped that we need to function".
> 
> The original xen_map_identity_early maps up to max_pfn, that is
> xen_start_info->nr_pages, so I am assuming that what you meant is that
> "Xen provides us with all the memory already mapped in the bootstrap
> page tables".
> And that is not written anywhere in the Xen headers.
> 
> Therefore, if I understand the issue correctly, I would add the
> following to xen.h:
> 
> "On x86_64 the bootstrap page tables map all the pages assigned to the
> domain."

That certainly is not the case (and can't be - remember that the
virtual space on x86-64 Linux'es initial mapping starts 2Gb from the
end of address space, so how could all memory possibly be mapped
[i.e. to what virtual addresses would those mappings be done]).

Jan

Re: [PATCH 3 of 4] xen/pagetables: Document that all of the initial regions are mapped

[Konrad Rzeszutek Wilk]

On Wed, Aug 22, 2012 at 04:35:56PM +0100, Jan Beulich wrote:
> >>> On 22.08.12 at 17:24, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> wrote:
> > On Wed, 22 Aug 2012, Konrad Rzeszutek Wilk wrote:
> >> On Wed, Aug 22, 2012 at 03:17:04PM +0100, Jan Beulich wrote:
> >> > >>> On 21.08.12 at 22:08, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> >> > > # HG changeset patch
> >> > > # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> > > # Date 1345579709 14400
> >> > > # Node ID 74bedb086c5b72447262e087c0218b89f8bc9140
> >> > > # Parent  8ed3eef706710c9c476a8d984bfb2861d92bedfb
> >> > > xen/pagetables: Document that all of the initial regions are mapped.
> >> > > 
> >> > > The documentation states that the layout of the initial region looks
> >> > > as so:
> >> > >    a. relocated kernel image
> >> > >    b. initial ram disk              [mod_start, mod_len]
> >> > >    c. list of allocated page frames [mfn_list, nr_pages]
> >> > >       (unless relocated due to XEN_ELFNOTE_INIT_P2M)
> >> > >    d. start_info_t structure        [register ESI (x86)]
> >> > >    e. bootstrap page tables         [pt_base, CR3 (x86)]
> >> > >    f. bootstrap stack               [register ESP (x86)]
> >> > > 
> >> > > But it does not clarify that the virtual address to all of
> >> > > those areas is initially mapped by the pt_base (or CR3).
> >> > > Lets fix that.
> >> > 
> >> > To me this is already being said by "This the order of bootstrap
> >> > elements in the initial virtual region".
> >> 
> >> Stefano wanted to make sure we have it written as clear as possible.
> >> I am going to be a good little submitter and let you guys sort this
> >> one out  :-)
> > 
> > Let's step back for a second and see if I understand correctly: your
> > patch 6/11 removes the call to xen_map_identity_early on x86_64 because
> > "Xen provides us with all the memory mapped that we need to function".

Hm, it should have said: "all the bootstrap memory we need.."
and perhaps include that it covers the kernel, ramdisk, p2m list
and the pagetables provided by the kernel.

> > 
> > The original xen_map_identity_early maps up to max_pfn, that is
> > xen_start_info->nr_pages, so I am assuming that what you meant is that
> > "Xen provides us with all the memory already mapped in the bootstrap
> > page tables".
> > And that is not written anywhere in the Xen headers.
> > 
> > Therefore, if I understand the issue correctly, I would add the
> > following to xen.h:
> > 
> > "On x86_64 the bootstrap page tables map all the pages assigned to the
> > domain."
> 
> That certainly is not the case (and can't be - remember that the
> virtual space on x86-64 Linux'es initial mapping starts 2Gb from the
> end of address space, so how could all memory possibly be mapped
> [i.e. to what virtual addresses would those mappings be done]).
> 
> Jan

[PATCH 4 of 4] xen/pagetables: Document pt_base inconsistency when running in COMPAT mode

[Konrad Rzeszutek Wilk]

# HG changeset patch
# User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
# Date 1345579709 14400
# Node ID b992f8e613a3401b9ddd140ce436c840d412beb7
# Parent  74bedb086c5b72447262e087c0218b89f8bc9140
xen/pagetables: Document pt_base inconsistency when running in COMPAT mode.

c/s 13257 added the COMPAT mode wherein a 64-bit hypervisor can
run with a 32-bit initial domain. One of the things it added was
that the pt_base is offset by two pages. This inconsistency
is only present in the COMPAT mode so lets document it.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff -r 74bedb086c5b -r b992f8e613a3 xen/include/public/xen.h
--- a/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
+++ b/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
@@ -663,7 +663,7 @@ typedef struct shared_info shared_info_t
  *      c. list of allocated page frames [mfn_list, nr_pages]
  *         (unless relocated due to XEN_ELFNOTE_INIT_P2M)
  *      d. start_info_t structure        [register ESI (x86)]
- *      e. bootstrap page tables         [pt_base, CR3 (x86)]
+ *      e. bootstrap page tables         [pt_base, CR3 (x86)] *1
  *      f. bootstrap stack               [register ESP (x86)]
  *  4. Bootstrap elements are packed together, but each is 4kB-aligned.
  *  5. The initial ram disk may be omitted.
@@ -678,6 +678,9 @@ typedef struct shared_info shared_info_t
  *
  *  NOTE: The initial virtual region (3a -> 3f) are all mapped by the initial
  *  pagetables [pt_base, CR3 (x86)].
+ *
+ *  *1: When booting under a 64-bit hypervisor with a 32-bit initial domain
+ *  it is offset by two pages (pt_base += PAGE_SIZE*2).
  */
 
 #define MAX_GUEST_CMDLINE 1024

Re: [PATCH 4 of 4] xen/pagetables: Document pt_base inconsistency when running in COMPAT mode

[Ian Campbell]

On Tue, 2012-08-21 at 21:08 +0100, Konrad Rzeszutek Wilk wrote:
> # HG changeset patch
> # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> # Date 1345579709 14400
> # Node ID b992f8e613a3401b9ddd140ce436c840d412beb7
> # Parent  74bedb086c5b72447262e087c0218b89f8bc9140
> xen/pagetables: Document pt_base inconsistency when running in COMPAT mode.
> 
> c/s 13257 added the COMPAT mode wherein a 64-bit hypervisor can
> run with a 32-bit initial domain. One of the things it added was
> that the pt_base is offset by two pages. This inconsistency
> is only present in the COMPAT mode so lets document it.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> diff -r 74bedb086c5b -r b992f8e613a3 xen/include/public/xen.h
> --- a/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
> +++ b/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
> @@ -663,7 +663,7 @@ typedef struct shared_info shared_info_t
>   *      c. list of allocated page frames [mfn_list, nr_pages]
>   *         (unless relocated due to XEN_ELFNOTE_INIT_P2M)
>   *      d. start_info_t structure        [register ESI (x86)]
> - *      e. bootstrap page tables         [pt_base, CR3 (x86)]
> + *      e. bootstrap page tables         [pt_base, CR3 (x86)] *1
>   *      f. bootstrap stack               [register ESP (x86)]
>   *  4. Bootstrap elements are packed together, but each is 4kB-aligned.
>   *  5. The initial ram disk may be omitted.
> @@ -678,6 +678,9 @@ typedef struct shared_info shared_info_t
>   *
>   *  NOTE: The initial virtual region (3a -> 3f) are all mapped by the initial
>   *  pagetables [pt_base, CR3 (x86)].
> + *
> + *  *1: When booting under a 64-bit hypervisor with a 32-bit initial domain
> + *  it is offset by two pages (pt_base += PAGE_SIZE*2).

"it" here being the page which you would have to load into cr3?

What, if anything, ends up in the other two pages?

Ian.

Re: [PATCH 4 of 4] xen/pagetables: Document pt_base inconsistency when running in COMPAT mode

[Konrad Rzeszutek Wilk]

On Wed, Aug 22, 2012 at 10:21:10AM +0100, Ian Campbell wrote:
> On Tue, 2012-08-21 at 21:08 +0100, Konrad Rzeszutek Wilk wrote:
> > # HG changeset patch
> > # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > # Date 1345579709 14400
> > # Node ID b992f8e613a3401b9ddd140ce436c840d412beb7
> > # Parent  74bedb086c5b72447262e087c0218b89f8bc9140
> > xen/pagetables: Document pt_base inconsistency when running in COMPAT mode.
> > 
> > c/s 13257 added the COMPAT mode wherein a 64-bit hypervisor can
> > run with a 32-bit initial domain. One of the things it added was
> > that the pt_base is offset by two pages. This inconsistency
> > is only present in the COMPAT mode so lets document it.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > diff -r 74bedb086c5b -r b992f8e613a3 xen/include/public/xen.h
> > --- a/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
> > +++ b/xen/include/public/xen.h	Tue Aug 21 16:08:29 2012 -0400
> > @@ -663,7 +663,7 @@ typedef struct shared_info shared_info_t
> >   *      c. list of allocated page frames [mfn_list, nr_pages]
> >   *         (unless relocated due to XEN_ELFNOTE_INIT_P2M)
> >   *      d. start_info_t structure        [register ESI (x86)]
> > - *      e. bootstrap page tables         [pt_base, CR3 (x86)]
> > + *      e. bootstrap page tables         [pt_base, CR3 (x86)] *1
> >   *      f. bootstrap stack               [register ESP (x86)]
> >   *  4. Bootstrap elements are packed together, but each is 4kB-aligned.
> >   *  5. The initial ram disk may be omitted.
> > @@ -678,6 +678,9 @@ typedef struct shared_info shared_info_t
> >   *
> >   *  NOTE: The initial virtual region (3a -> 3f) are all mapped by the initial
> >   *  pagetables [pt_base, CR3 (x86)].
> > + *
> > + *  *1: When booting under a 64-bit hypervisor with a 32-bit initial domain
> > + *  it is offset by two pages (pt_base += PAGE_SIZE*2).
> 
> "it" here being the page which you would have to load into cr3?

No. The pt_base value.
> 
> What, if anything, ends up in the other two pages?

The L3 and L1. The pt_base has incorrect data and points to second L1 instead
of the L3.

Its laid out like this: [L3] [L1] [L1] ... [L2]
                         a    b    c

and pt_base has the phys_addr of 'c' instead of 'a'.

Re: [PATCH 4 of 4] xen/pagetables: Document pt_base inconsistency when running in COMPAT mode

[Jan Beulich]

>>> On 21.08.12 at 22:08, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> # HG changeset patch
> # User Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> # Date 1345579709 14400
> # Node ID b992f8e613a3401b9ddd140ce436c840d412beb7
> # Parent  74bedb086c5b72447262e087c0218b89f8bc9140
> xen/pagetables: Document pt_base inconsistency when running in COMPAT mode.
> 
> c/s 13257 added the COMPAT mode wherein a 64-bit hypervisor can
> run with a 32-bit initial domain. One of the things it added was
> that the pt_base is offset by two pages. This inconsistency
> is only present in the COMPAT mode so lets document it.

Let's first understand whether that's really a bug.

Jan