Re: [PATCH 7/8] docs: Document start_info changes in Xen 4.2.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Jan 29, 2013 at 09:37:00AM +0000, Ian Campbell wrote:
> On Mon, 2013-01-28 at 18:32 +0000, Konrad Rzeszutek Wilk wrote:
> > The git commit 7a9d7646307b7c872b8dbd7546579acd3b54223d "x86/32-on-64:
> > adjust Dom0 initial page table layout" fixes a bug in the reported
> > value of pt_base versus what is stored in the %cr3 register. This
> > documents this in the start of the world header note.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >  xen/include/public/xen.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
> > index c50bbfc..1685317 100644
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -705,6 +705,13 @@ 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: Prior to 7a9d7646307b7c872b8dbd7546579acd3b54223d ("x86/32-on-64:
> > + * adjust Dom0 initial page table layout") the 3.e) contained two different
> > + * values with a 64-bit hypervisor and a 32-bit initial domain kernel. The
> > + * pt_base pointed to the L4 (setup by the hypervisor and not used by
> > + * the guest) and the %cr3 pointed to the L3. This meant an difference of
> > + * one page.
> >   */
> 
> 7a9d7646307b7c872b8dbd7546579acd3b54223d is not a Xen revision. I think
> you meant 25833:bb85bbccb1c9. I assume this was a bug fix, in which case
> I would say "Prior to Xen commit xxx:yyyy ("x86/32-on...") a bug caused
> the pt_base (3.e above) to contain ..."
> 
> It would be useful to explain clearly the after case too, which I
> suppose is that on 32-on-64 the pt_base now points to what the guest
> kernel should consider to be the base? Would it be relevant to explain
> how to detect this situation and what to do about it?
> 
> The difference of one page was just an implementation artefact, rather
> than a meaningful semantic difference? Unless it is relevant to
> detecting the situation I would be inclined not to do it.

It can be construed either way. If you look at:

 *      e. bootstrap page tables         [pt_base, CR3 (x86)]                   

It mentions _both_ sources of information - the register and the
contents of pt_base. It could imply that both of them have the same
value (if you read that as an enumeration of where this value is
contained). But if you read the ',' as an 'or', then it is not a
semantic diference as they both could contain drastically diferent
values.

Implementation wise, we get is that the value that CR3 is different than what:
    unsigned long pt_base;      /* VIRTUAL address of page directory.  */   

has. In other words, the pt_base is suspect and should be carefully
compared to the %cr3. If they are different, then the user should _NOT_
touch the pages in between pt_base and %cr3 and mark them as unusable.

> 
> It should also be made clearer that this affected only 32-on-64 dom0
> kernels and not 32-on-64 domU or 64-bit kernels of any colour.

Right. How about this:

commit 489682893ffc8bbbbb12ee820defac17cce762d0
Author: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date:   Mon Jan 28 13:20:29 2013 -0500

    docs: Document start_info changes in Xen 4.2.
    
    The  25833:bb85bbccb1c9. "x86/32-on-64: adjust Dom0 initial page table layout"
    fixes a bug in the reported value of pt_base versus what is stored in
    the %cr3 register. This documents this in the start of the world header note.
    
    Also make it crystal clear that pt_base == %cr3.
    
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index c50bbfc..3f51cd6 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -693,7 +693,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 and CR3 (x86)]
  *      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.
@@ -705,6 +705,15 @@ 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: Prior to 25833:bb85bbccb1c9. ("x86/32-on-64 adjust Dom0 initial page
+ * table layout") a bug caused the pt_base (3.e above) to contain a different
+ * value than the CR3 register. This only manifested itself on 32-on-64 dom0
+ * kernels and not 32-on-64 domU or 64-bit kernels of any colour. The
+ * pt_base pointed to the L4 (setup by the hypervisor and not used by
+ * the guest) and the %cr3 pointed to the L3. This meant an difference of
+ * one page. The guest should _NOT_ use the pages from pt_base to %cr3
+ * for anything and mark them as reserved/unused.
  */
 
 #define MAX_GUEST_CMDLINE 1024