|
On 11/07/13 03:35, Jan Beulich wrote:
On 06.11.13 at 21:08, Don Slutz <dslutz@xxxxxxxxxxx> wrote:
@@ -76,22 +86,28 @@ unsigned long long kernel_start = 0xc0000000;
unsigned long long kernel_start = 0xffffffff80000000UL;
#endif
-static int is_kernel_text(guest_word_t addr)
+static type_of_addr is_kernel_addr(guest_word_t addr)
The "is_" prefix is now bogus.
Ok, will remove.
{
- if (symbol_table == NULL)
- return (addr > kernel_start);
+ if (symbol_table == NULL) {
+ if (addr > kernel_start)
+ return KERNEL_TEXT_ADDR;
+ else
+ return NOT_KERNEL_ADDR;
+ }
if (addr >= kernel_stext &&
addr <= kernel_etext)
- return 1;
- if (kernel_hypercallpage &&
- (addr >= kernel_hypercallpage &&
- addr <= kernel_hypercallpage + 4096))
- return 1;
+ return KERNEL_TEXT_ADDR;
+ if (kernel_hypercallpage && (addr >= kernel_hypercallpage &&
+ addr <= kernel_hypercallpage + 4096))
This reformatting is pointlessly bloating the patch - if you want
the line reformatted, you should (a) do this in the patch that
adds the first part of the condition and (b) obey to indentation
rules.
Opps, I did not what this reformatting, will just drop it.
+ return KERNEL_TEXT_ADDR;
if (addr >= kernel_sinittext &&
addr <= kernel_einittext)
- return 1;
- return 0;
+ return KERNEL_TEXT_ADDR;
+ if (addr >= kernel_text &&
+ addr <= kernel_end)
+ return KERNEL_DATA_ADDR;
As you supposedly filtered out all text ranges before, did you really
mean to compare to kernel_text here (rather than kernel_start)?
Yes, I did. I think it is better to use the value that is in the
system map over the default. It has changed:
dcs-xen-54:~/xen>grep " _text" /boot/System.map-*
/boot/System.map-2.6.18-128.el5:ffffffff80000000 A _text
/boot/System.map-3.6.11-5.fc17.x86_64:ffffffff81000000 T _text
/boot/System.map-3.8.11-100.fc17.x86_64:ffffffff81000000 T _text
But since it is a command line argument, if specified, should it be
used instead?
@@ -749,7 +769,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
void *p = map_page(ctx, vcpu, stack);
if (!p)
return -1;
- word = read_stack_word(p, width);
+ word = read_mem_word(ctx, vcpu, stack, width);
How is this change related to the purpose of the patch?
Opps, wrong split. This (and the one 2 below changing to
read_mem_word) should be in their own patch (or back in patch 8,
with patch 9 before 8. Without this change specifying an unaligned
address can read across a page boundary which would not get he
correct data (and can fault).
@@ -818,7 +838,7 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
if (xenctx.stack_trace) {
print_stack_word(stack, width);
printf(": |-- ");
- print_stack_word(read_stack_word(p, width), width);
+ print_stack_word(frame, width);
And this one?
This cleanup mostly likely should be just dropped (or made it's own
patch). The context is just one line short:
frame = read_stack_word(p, width);
if (xenctx.stack_trace) {
...
print_stack_word(read_stack_word(p, width), width);
And since read_stack_word(p, width) should always return the same
data for the same address; the 2nd call is not needed.
@@ -843,13 +863,13 @@ static int print_stack(vcpu_guest_context_any_t *ctx, int vcpu, int width, guest
void *p = map_page(ctx, vcpu, stack);
if (!p)
return -1;
- word = read_stack_word(p, width);
- if (is_kernel_text(word)) {
+ word = read_mem_word(ctx, vcpu, stack, width);
+ if (is_kernel_addr(word) >= KERNEL_TEXT_ADDR) {
And one more.
The 2nd line is needed is_kernel_text() changed to is_kernel_addr().
-Don Slutz
Jan
|
