[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v2 10/12] xenctx: change is_kernel_text() into is_kernel_addr().



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


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.