From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v2 10/12] xenctx: change is_kernel_text() into is_kernel_addr(). Date: Thu, 7 Nov 2013 19:51:38 -0500 Message-ID: <527C359A.5090301@terremark.com> References: <1383768500-4245-1-git-send-email-dslutz@terremark.com> <1383768500-4245-11-git-send-email-dslutz@terremark.com> <527B5EEC02000078001007BD@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4823779257330725296==" Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VeaR4-0002mr-MP for xen-devel@lists.xenproject.org; Fri, 08 Nov 2013 01:00:31 +0000 In-Reply-To: <527B5EEC02000078001007BD@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Don Slutz Cc: Ian Campbell , Stefano Stabellini , George Dunlap , Don Slutz , Ian Jackson , xen-devel List-Id: xen-devel@lists.xenproject.org --===============4823779257330725296== Content-Type: multipart/alternative; boundary="------------090402060207030405000404" --------------090402060207030405000404 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit On 11/07/13 03:35, Jan Beulich wrote: >>>> On 06.11.13 at 21:08, Don Slutz 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 > --------------090402060207030405000404 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 11/07/13 03:35, Jan Beulich wrote:
On 06.11.13 at 21:08, Don Slutz <dslutz@verizon.com> 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



    

    

  


--------------090402060207030405000404--


--===============4823779257330725296==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

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

--===============4823779257330725296==--