* [PATCH 2/3] xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor
@ 2012-01-20 18:45 Marcus Granado
2012-01-23 9:50 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Marcus Granado @ 2012-01-20 18:45 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap
[-- Attachment #1: Type: text/plain, Size: 2885 bytes --]
xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor
The dump_guest_backtrace() function attempted to walk the stack
based on the assumption that the guest and hypervisor pointer sizes
were the same; thus any 32-bit guest running under 64-bit hypervisor
would have unreliable results.
In 64-bit mode, read the 32-bit stack frame properly.
Signed-off-by: Marcus Granado <marcus.granado@eu.citrix.com>
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
diff -r f6953e89913f -r 21e7744a52b1 xen/arch/x86/oprofile/backtrace.c
--- a/xen/arch/x86/oprofile/backtrace.c Wed Jan 18 17:23:02 2012 +0000
+++ b/xen/arch/x86/oprofile/backtrace.c Wed Jan 18 17:35:06 2012 +0000
@@ -20,6 +20,13 @@ struct frame_head {
unsigned long ret;
} __attribute__((packed));
+#ifdef CONFIG_X86_64
+struct frame_head_32bit {
+ uint32_t ebp;
+ unsigned long ret;
+} __attribute__((packed));
+#endif
+
static struct frame_head *
dump_hypervisor_backtrace(struct domain *d, struct vcpu *vcpu,
struct frame_head * head, int mode)
@@ -35,19 +42,46 @@ dump_hypervisor_backtrace(struct domain
return head->ebp;
}
+#ifdef CONFIG_X86_64
+static inline int is_32bit_vcpu(struct vcpu *vcpu)
+{
+ if (is_hvm_vcpu(vcpu))
+ return !hvm_long_mode_enabled(vcpu);
+ else
+ return is_pv_32bit_vcpu(vcpu);
+}
+#endif
+
static struct frame_head *
dump_guest_backtrace(struct domain *d, struct vcpu *vcpu,
struct frame_head * head, int mode)
{
struct frame_head bufhead[2];
XEN_GUEST_HANDLE(char) guest_head = guest_handle_from_ptr(head, char);
-
- /* Also check accessibility of one struct frame_head beyond */
- if (!guest_handle_okay(guest_head, sizeof(bufhead)))
- return 0;
- if (__copy_from_guest_offset((char *)bufhead, guest_head, 0,
- sizeof(bufhead)))
- return 0;
+
+#ifdef CONFIG_X86_64
+ if ( is_32bit_vcpu(vcpu) )
+ {
+ struct frame_head_32bit bufhead32[2];
+ /* Also check accessibility of one struct frame_head beyond */
+ if (!guest_handle_okay(guest_head, sizeof(bufhead32)))
+ return 0;
+ if (__copy_from_guest_offset((char *)bufhead32, guest_head, 0,
+ sizeof(bufhead32)))
+ return 0;
+ bufhead[0].ebp=(struct frame_head *)(unsigned
long)bufhead32[0].ebp;
+ bufhead[0].ret=bufhead32[0].ret;
+ }
+ else
+#endif
+ {
+ /* Also check accessibility of one struct frame_head beyond */
+ if (!guest_handle_okay(guest_head, sizeof(bufhead)))
+ return 0;
+ if (__copy_from_guest_offset((char *)bufhead, guest_head, 0,
+ sizeof(bufhead)))
+ return 0;
+ }
if (!xenoprof_add_trace(d, vcpu, bufhead[0].ret, mode))
return 0;
[-- Attachment #2: xenoprof-handle-32-bit-guest-stacks.diff --]
[-- Type: text/x-patch, Size: 2861 bytes --]
xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor
The dump_guest_backtrace() function attempted to walk the stack
based on the assumption that the guest and hypervisor pointer sizes
were the same; thus any 32-bit guest running under 64-bit hypervisor
would have unreliable results.
In 64-bit mode, read the 32-bit stack frame properly.
Signed-off-by: Marcus Granado <marcus.granado@eu.citrix.com>
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
diff -r f6953e89913f -r 21e7744a52b1 xen/arch/x86/oprofile/backtrace.c
--- a/xen/arch/x86/oprofile/backtrace.c Wed Jan 18 17:23:02 2012 +0000
+++ b/xen/arch/x86/oprofile/backtrace.c Wed Jan 18 17:35:06 2012 +0000
@@ -20,6 +20,13 @@ struct frame_head {
unsigned long ret;
} __attribute__((packed));
+#ifdef CONFIG_X86_64
+struct frame_head_32bit {
+ uint32_t ebp;
+ unsigned long ret;
+} __attribute__((packed));
+#endif
+
static struct frame_head *
dump_hypervisor_backtrace(struct domain *d, struct vcpu *vcpu,
struct frame_head * head, int mode)
@@ -35,19 +42,46 @@ dump_hypervisor_backtrace(struct domain
return head->ebp;
}
+#ifdef CONFIG_X86_64
+static inline int is_32bit_vcpu(struct vcpu *vcpu)
+{
+ if (is_hvm_vcpu(vcpu))
+ return !hvm_long_mode_enabled(vcpu);
+ else
+ return is_pv_32bit_vcpu(vcpu);
+}
+#endif
+
static struct frame_head *
dump_guest_backtrace(struct domain *d, struct vcpu *vcpu,
struct frame_head * head, int mode)
{
struct frame_head bufhead[2];
XEN_GUEST_HANDLE(char) guest_head = guest_handle_from_ptr(head, char);
-
- /* Also check accessibility of one struct frame_head beyond */
- if (!guest_handle_okay(guest_head, sizeof(bufhead)))
- return 0;
- if (__copy_from_guest_offset((char *)bufhead, guest_head, 0,
- sizeof(bufhead)))
- return 0;
+
+#ifdef CONFIG_X86_64
+ if ( is_32bit_vcpu(vcpu) )
+ {
+ struct frame_head_32bit bufhead32[2];
+ /* Also check accessibility of one struct frame_head beyond */
+ if (!guest_handle_okay(guest_head, sizeof(bufhead32)))
+ return 0;
+ if (__copy_from_guest_offset((char *)bufhead32, guest_head, 0,
+ sizeof(bufhead32)))
+ return 0;
+ bufhead[0].ebp=(struct frame_head *)(unsigned long)bufhead32[0].ebp;
+ bufhead[0].ret=bufhead32[0].ret;
+ }
+ else
+#endif
+ {
+ /* Also check accessibility of one struct frame_head beyond */
+ if (!guest_handle_okay(guest_head, sizeof(bufhead)))
+ return 0;
+ if (__copy_from_guest_offset((char *)bufhead, guest_head, 0,
+ sizeof(bufhead)))
+ return 0;
+ }
if (!xenoprof_add_trace(d, vcpu, bufhead[0].ret, mode))
return 0;
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor
2012-01-20 18:45 [PATCH 2/3] xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor Marcus Granado
@ 2012-01-23 9:50 ` Jan Beulich
2012-01-24 19:27 ` Marcus Granado
0 siblings, 1 reply; 4+ messages in thread
From: Jan Beulich @ 2012-01-23 9:50 UTC (permalink / raw)
To: Marcus Granado; +Cc: George Dunlap, xen-devel
>>> On 20.01.12 at 19:45, Marcus Granado <marcus.granado@citrix.com> wrote:
> xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor
>
> The dump_guest_backtrace() function attempted to walk the stack
> based on the assumption that the guest and hypervisor pointer sizes
> were the same; thus any 32-bit guest running under 64-bit hypervisor
> would have unreliable results.
>
> In 64-bit mode, read the 32-bit stack frame properly.
>
> Signed-off-by: Marcus Granado <marcus.granado@eu.citrix.com>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>
>
> diff -r f6953e89913f -r 21e7744a52b1 xen/arch/x86/oprofile/backtrace.c
> --- a/xen/arch/x86/oprofile/backtrace.c Wed Jan 18 17:23:02 2012 +0000
> +++ b/xen/arch/x86/oprofile/backtrace.c Wed Jan 18 17:35:06 2012 +0000
> @@ -20,6 +20,13 @@ struct frame_head {
> unsigned long ret;
> } __attribute__((packed));
>
> +#ifdef CONFIG_X86_64
> +struct frame_head_32bit {
> + uint32_t ebp;
> + unsigned long ret;
unsigned long (i.e. 64 bits)?
> +} __attribute__((packed));
> +#endif
> +
> static struct frame_head *
> dump_hypervisor_backtrace(struct domain *d, struct vcpu *vcpu,
> struct frame_head * head, int mode)
> @@ -35,19 +42,46 @@ dump_hypervisor_backtrace(struct domain
> return head->ebp;
> }
>
> +#ifdef CONFIG_X86_64
> +static inline int is_32bit_vcpu(struct vcpu *vcpu)
> +{
> + if (is_hvm_vcpu(vcpu))
> + return !hvm_long_mode_enabled(vcpu);
> + else
> + return is_pv_32bit_vcpu(vcpu);
> +}
> +#endif
> +
> static struct frame_head *
> dump_guest_backtrace(struct domain *d, struct vcpu *vcpu,
> struct frame_head * head, int mode)
> {
> struct frame_head bufhead[2];
> XEN_GUEST_HANDLE(char) guest_head = guest_handle_from_ptr(head, char);
> -
> - /* Also check accessibility of one struct frame_head beyond */
> - if (!guest_handle_okay(guest_head, sizeof(bufhead)))
> - return 0;
> - if (__copy_from_guest_offset((char *)bufhead, guest_head, 0,
> - sizeof(bufhead)))
> - return 0;
> +
> +#ifdef CONFIG_X86_64
> + if ( is_32bit_vcpu(vcpu) )
> + {
> + struct frame_head_32bit bufhead32[2];
> + /* Also check accessibility of one struct frame_head beyond */
> + if (!guest_handle_okay(guest_head, sizeof(bufhead32)))
> + return 0;
> + if (__copy_from_guest_offset((char *)bufhead32, guest_head, 0,
If you're adding a compat mode guest case here, then you should
also use compat mode accessors (compat_handle_okay(),
__copy_from_compat_offset()), implying that you also have a local
handle variable of the appropriate type (and perhaps moving the
native one down into the 'else' body).
Also, as you're touching this code anyway, the
__copy_from_..._offset() here and below, as they're passing literal
zero for the offset, can be abbreviated by using __copy_from_...()
(i.e. without the offset).
Jan
> + sizeof(bufhead32)))
> + return 0;
> + bufhead[0].ebp=(struct frame_head *)(unsigned long)bufhead32[0].ebp;
> + bufhead[0].ret=bufhead32[0].ret;
> + }
> + else
> +#endif
> + {
> + /* Also check accessibility of one struct frame_head beyond */
> + if (!guest_handle_okay(guest_head, sizeof(bufhead)))
> + return 0;
> + if (__copy_from_guest_offset((char *)bufhead, guest_head, 0,
> + sizeof(bufhead)))
> + return 0;
> + }
>
> if (!xenoprof_add_trace(d, vcpu, bufhead[0].ret, mode))
> return 0;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor
2012-01-23 9:50 ` Jan Beulich
@ 2012-01-24 19:27 ` Marcus Granado
2012-01-25 7:52 ` Jan Beulich
0 siblings, 1 reply; 4+ messages in thread
From: Marcus Granado @ 2012-01-24 19:27 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, xen-devel
On 23/01/12 09:50, Jan Beulich wrote:
> If you're adding a compat mode guest case here, then you should
> also use compat mode accessors (compat_handle_okay(),
> __copy_from_compat_offset()), implying that you also have a local
> handle variable of the appropriate type (and perhaps moving the
> native one down into the 'else' body).
I'm trying to understand the compat handle. It is not clear to me how to
map one from head (a 64-bit pointer), since COMPAT_HANDLE seems to store
a 32-bit compat_ptr_t value in its structure. Ideally, what I would like
to do is
COMPAT_HANDLE(char) guest_head = map_guest_handle_to_compat_handle
(guest_handle_from_ptr(head, char));
or
COMPAT_HANDLE(char) guest_head = compat_handle_from_ptr(head, char));
but I can't find any equivalent functions in any header.
The following line compiles,
COMPAT_HANDLE(char) guest_head = { (full_ptr_t)head };
but it looks like, in this case, the compat handle structure in compat.h
will truncate the most significant bits from the head pointer, so
compat_handle_okay(guest_head,...) and
__copy_from_compat(...,guest_head,...) below will be using a truncated
pointer:
56 static struct frame_head *
57 dump_guest_backtrace(struct domain *d, struct vcpu *vcpu,
58 struct frame_head * head, int mode)
59 {
60 struct frame_head bufhead[2];
61
62 #ifdef CONFIG_X86_64
63 if ( is_32bit_vcpu(vcpu) )
64 {
65 COMPAT_HANDLE(char) guest_head = { (full_ptr_t)head };
66 struct frame_head_32bit bufhead32[2];
67 /* Also check accessibility of one struct frame_head
beyond */
68 if (!compat_handle_okay(guest_head, sizeof(bufhead32)))
69 return 0;
70 if (__copy_from_compat((char *)bufhead32, guest_head,
71 sizeof(bufhead32)))
72 return 0;
73 bufhead[0].ebp=(struct frame_head
*)(full_ptr_t)bufhead32[0].ebp;
74 bufhead[0].ret=bufhead32[0].ret;
75 }
76 else
77 #endif
Any advice? Maybe the best option in this case is to avoid the compat*
functions and to use the original guest* functions instead.
Thanks,
Marcus
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 2/3] xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor
2012-01-24 19:27 ` Marcus Granado
@ 2012-01-25 7:52 ` Jan Beulich
0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2012-01-25 7:52 UTC (permalink / raw)
To: Marcus Granado; +Cc: George Dunlap, xen-devel
>>> On 24.01.12 at 20:27, Marcus Granado <marcus.granado@citrix.com> wrote:
> I'm trying to understand the compat handle. It is not clear to me how to
> map one from head (a 64-bit pointer), since COMPAT_HANDLE seems to store
That cannot generally be done, as a 64-bit pointer can never be
represented as a compat handle.
Proper conversion has to start at where 'head' is first generated (i.e.
the line
head = (struct frame_head *)regs->ebp;
in xenoprof_backtrace() (the more that here you really *want* to
drop the upper 32 bits in the compat case. Working with a union is
a possible approach, but it may also be acceptable to actually do
the truncation in dump_guest_backtrace(), properly explaining why
the dropping of the upper half is valid and intended there.
(I've already put fixing up of this already committed patch on my
todo list, so feel free to drop further attempts; once I'm done I'd
appreciate review/testing of the code though.)
Jan
> a 32-bit compat_ptr_t value in its structure. Ideally, what I would like
> to do is
>
> COMPAT_HANDLE(char) guest_head = map_guest_handle_to_compat_handle
> (guest_handle_from_ptr(head, char));
> or
> COMPAT_HANDLE(char) guest_head = compat_handle_from_ptr(head, char));
> but I can't find any equivalent functions in any header.
>
> The following line compiles,
> COMPAT_HANDLE(char) guest_head = { (full_ptr_t)head };
> but it looks like, in this case, the compat handle structure in compat.h
> will truncate the most significant bits from the head pointer, so
> compat_handle_okay(guest_head,...) and
> __copy_from_compat(...,guest_head,...) below will be using a truncated
> pointer:
>
> 56 static struct frame_head *
> 57 dump_guest_backtrace(struct domain *d, struct vcpu *vcpu,
> 58 struct frame_head * head, int mode)
> 59 {
> 60 struct frame_head bufhead[2];
> 61
> 62 #ifdef CONFIG_X86_64
> 63 if ( is_32bit_vcpu(vcpu) )
> 64 {
> 65 COMPAT_HANDLE(char) guest_head = { (full_ptr_t)head };
> 66 struct frame_head_32bit bufhead32[2];
> 67 /* Also check accessibility of one struct frame_head
> beyond */
> 68 if (!compat_handle_okay(guest_head, sizeof(bufhead32)))
> 69 return 0;
> 70 if (__copy_from_compat((char *)bufhead32, guest_head,
> 71 sizeof(bufhead32)))
> 72 return 0;
> 73 bufhead[0].ebp=(struct frame_head
> *)(full_ptr_t)bufhead32[0].ebp;
> 74 bufhead[0].ret=bufhead32[0].ret;
> 75 }
> 76 else
> 77 #endif
>
> Any advice? Maybe the best option in this case is to avoid the compat*
> functions and to use the original guest* functions instead.
>
> Thanks,
> Marcus
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-01-25 7:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-20 18:45 [PATCH 2/3] xenoprof: Handle 32-bit guest stacks properly in a 64-bit hypervisor Marcus Granado
2012-01-23 9:50 ` Jan Beulich
2012-01-24 19:27 ` Marcus Granado
2012-01-25 7:52 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).