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

[Don Slutz <dslutz@verizon.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4255 bytes --]

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
>


[-- Attachment #1.2: Type: text/html, Size: 6668 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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