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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-01-30 at 17:23 +0000, Konrad Rzeszutek Wilk wrote:
> 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@xxxxxxxxxx>
> > > ---
> > >  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).

This comment is written from the context of a 32-bit hypervisor, where
this would be true. I think it's just become inaccurate on 32-on-64 due
to the trick we play with grafting the guest 3-level tables into a
4-level table.

>  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@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> 
> 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.

I don't think this is right, the bug was that the L4 table was in guest
space at all and that pt_base pointed to it, wasn't it? The fix was to
remove the L4 from guest space altogether,

Remember that this is a 32-on-64 guest so the 64-bit hypervisor should
be providing the illusion of running on the 32-bit hypervisor, at least
so far as it is able (and perhaps the guest reading %cr3 direct is one
of those places where it can't)

Perhaps Jan can correct our misconceptions on this one.

>  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.

That sounds backwards, if actual %cr3 pointed to L3 things wouldn't work
-- the processor is in long mode (on a compat segment) and so needs 4
level pages.

>  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



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