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

[Hollis Blanchard <hollisb@xxxxxxxxxx>]

On Thu, 2006-05-11 at 17:02 +0200, Simon Kagstrom wrote:
> Debugging with the gdbserver has been broken for paravirtualized
> guests for a while since parts of xc_ptrace.c assumes HVM guests. The
> following patch fixes this along with single-stepping in attached
> domains. The patch tries to separate HVM-specific address
> handling. Changes:
> 
> - Functions va_to_ma32 and va_to_ma64 have been added. These translate
>   virtual to machine addresses through a given page directory/table
>   and optionally through page_array for HVM guests. These have
>   replaced page_array-references.
> 
> - paging_enabled now contains a check for HVM guests, and always
>   returns 1 for PV guests.
> 
> - Reversed logic in PTRACE_SINGLESTEP to allow stepping in attached
>   domains and disallow stepping in corefiles.
> 
> NOTE: I only have access to "regular" 32-bit hardware, so I've only
> been able to test map_domain_va32 for PV guests. It would be nice if
> other people could test it on HVM, PAE and 64-bit guests.
> 
> // Simon
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@xxxxxx>
> 
> ===File /tmp/xc_ptrace.patch================================
> diff -r fc9ec6fd3400 tools/libxc/xc_ptrace.c
> --- a/tools/libxc/xc_ptrace.c Mon May 08 14:56:18 2006 +0100
> +++ b/tools/libxc/xc_ptrace.c Thu May 11 16:49:59 2006 +0200
> @@ -100,8 +100,15 @@ static inline int
>  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.

>  /*
> @@ -157,6 +164,43 @@ static long                     nr_pages
>  static long                     nr_pages = 0;
>  static unsigned long           *page_array = NULL;
>  
> +
> +static unsigned long
> +va_to_ma32(int cpu,
> +         uint32_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)
> +        return 0;
> +    if ( (ctxt[cpu].flags & VGCF_HVM_GUEST) && paging_enabled(&ctxt[cpu]) )
> +        out = page_array[idx] << PAGE_SHIFT;
> +    return out;
> +}
> +
> +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?

> +    if ( (ctxt[cpu].flags & VGCF_HVM_GUEST) && paging_enabled(&ctxt[cpu]) )
> +        out = page_array[idx] << PAGE_SHIFT;
> +    return out;
> +}

I'd suggest renaming "out" to be more descriptive, like "maddr".

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

-- 
Hollis Blanchard
IBM Linux Technology Center


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