Re: [Xen-devel] [PATCH] reenabling ptrace for paravirtualized guests

[Simon Kagstrom <simon.kagstrom@xxxxxx>]

Thanks for your comments!

At Thu, 11 May 2006 16:29:57 -0500,
Hollis Blanchard wrote:
> >  static inline int 
> >  paging_enabled(vcpu_guest_context_t *v)
> >  {
> > -    unsigned long cr0 = v->ctrlreg[0];
> > -    return (cr0 & X86_CR0_PE) && (cr0 & X86_CR0_PG);
> > +    /* This check can be removed when Xen places the correct values in
> > +     * cr0 for paravirtualized guests.
> > +     */
> > +    if ( (v->flags & VGCF_HVM_GUEST) == 1 ) {
> > +            unsigned long cr0 = v->ctrlreg[0];
> > +
> > +            return (cr0 & X86_CR0_PE) && (cr0 & X86_CR0_PG);
> > +    }
> > +    return 1;
> >  }
> 
> Instead of this, please look at the patch Ryan sent (Re: [Xen-devel]
> [PATCH] paging_enabled and non-HVM guests)? You'll need to clean up the
> whitespace at least though.

I'll comment Ryans patch below too make sure I understand what you
mean.

> diff -r c4eead8a925b tools/libxc/xc_ptrace.c
> --- a/tools/libxc/xc_ptrace.c Sun Apr 16 14:41:31 2006
> +++ b/tools/libxc/xc_ptrace.c Thu Apr 20 22:44:35 2006
> @@ -281,8 +281,10 @@
>      uint64_t *l4, *l3, *l2, *l1;
>      static void *v;
>  
> +#if 0
>      if ((ctxt[cpu].ctrlreg[4] & 0x20) == 0 ) /* legacy ia32 mode */
>          return map_domain_va_32(xc_handle, cpu, guest_va, perm);
> +#endif

Is this check valid for non-HVM guests, i.e., is it possible to have
32-bit PV-guests on a 64-bit hypervisor?
 
> [3 setup_sane_cr0.patch <text/plain; us-ascii (7bit)>]
> diff -r c4eead8a925b tools/libxc/xc_linux_build.c
> --- a/tools/libxc/xc_linux_build.c    Sun Apr 16 14:41:31 2006
> +++ b/tools/libxc/xc_linux_build.c    Thu Apr 20 22:45:21 2006
> @@ -45,6 +45,11 @@
>  #ifdef __ia64__
>  #define probe_aout9(image,image_size,load_funcs) 1
>  #endif
> +
> +/* from xc_ptrace.h */
> +#define X86_CR0_PE              0x00000001 /* Enable Protected Mode    (RW) 
> */
> +#define X86_CR0_PG              0x80000000 /* Paging                   (RW) 
> */
> +
>  
>  struct initrd_info {
>      enum { INITRD_none, INITRD_file, INITRD_mem } type;
> @@ -1174,6 +1179,8 @@
>      ctxt->failsafe_callback_eip = 0;
>      ctxt->syscall_callback_eip  = 0;
>  #endif
> +    /* set sane cr0 bits, protected and paging enabled  */
> +    ctxt->ctrlreg[0] = X86_CR0_PE|X86_CR0_PG;
>  #endif /* x86 */
> [...]
> --- a/linux-2.6-xen-sparse/drivers/xen/core/smpboot.c Sun Apr 16 14:41:31 2006
> +++ b/linux-2.6-xen-sparse/drivers/xen/core/smpboot.c Thu Apr 20 22:45:36 2006
> @@ -216,6 +216,8 @@
>  
>       ctxt.gs_base_kernel = (unsigned long)(cpu_pda(vcpu));
>  #endif
> +   /* set sane cr0 bits, protected and paging enabled  */
> +   ctxt.ctrlreg[0] = 0x80000001;
>  
>       BUG_ON(HYPERVISOR_vcpu_op(VCPUOP_initialise, vcpu, &ctxt));
>  }

I'll add this to the patch.

> >  /*
> > @@ -157,6 +164,43 @@ static long                     nr_pages
> >  static long                     nr_pages = 0;
> >  static unsigned long           *page_array = NULL;
> >  
> > +static unsigned long
> > +va_to_ma64(int cpu,
> > +         uint64_t *table,
> > +         unsigned long idx)
> > +{
> > +    unsigned long out;
> > +
> > +    /* Paravirtualized domains store machine addresses in tables while
> > +     * HVM domains keep pseudo-physical addresses. HVM domains
> > +     * therefore need one extra translation.
> > +     */
> > +    if ( (out = table[idx]) == 0)
> 
> Isn't 0 as a physical or machine address valid?
> 
> >+        return 0;
> 
> Couldn't 0 be a valid machine address?
> 
> Can you leave these checks and the table indexing to the callers, where
> they are now?

True, I'll just return the value and revert the table indexing. I'm a
bit unsure about the checks-for-0 though. map_domain_va_32 does this,
but map_domain_va_pae and map_domain_va_64 contained no checks. Maybe
the proper fix is just to remove the checks in map_domain_va_32 as 0
can be a valid physical/machine address?

> Since you don't have an HVM system to test on, you should probably CC
> the people who do who've worked on xc_ptrace.c. hg log suggests Kamble,
> Nitin A <nitin.a.kamble@xxxxxxxxx>.

I'll send a reworked patch. Nitin: could you test the next patch?

// Simon

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel