[Crash-utility] [PATCH] Support cpu_map/cpu_mask changes in 2.6.29

[Michael Holzheu]

Hi Dave,

Am Dienstag, den 28.04.2009, 14:24 -0400 schrieb Dave Anderson:
> 
> Michael,
> 
> It's not clear to me how this function works:
> 
> + * Get cpu map (possible, online, etc.) address from cpu mask (since 2.6.29)
> + */
> +static ulong
> +get_cpu_map_addr_from_mask(const char *type)
> +{
> +       ulong cpu_mask_addr, cpu_map_addr;
> +       char cpu_mask_symbol[32];
> +       char *cpu_mask_buf;
> +       int cpu_mask_size;
> +
> +       sprintf(cpu_mask_symbol, "cpu_%s_mask", type);
> +
> +       if (!symbol_exists(cpu_mask_symbol))
> +               return 0;
> +
> +       cpu_mask_addr = symbol_value(cpu_mask_symbol);
> +       cpu_mask_size = STRUCT_SIZE("cpumask");
> +       cpu_mask_buf = malloc(cpu_mask_size);
> +       if (!cpu_mask_buf)
> +               error(FATAL, "get_cpu_map_addr_from_mask: out of memory\n");
> +       readmem(cpu_mask_addr, KVADDR, cpu_mask_buf, cpu_mask_size,
> +               cpu_mask_symbol, FAULT_ON_ERROR);
> +       cpu_map_addr = ULONG(cpu_mask_buf + MEMBER_OFFSET("cpumask", "bits"));
> +       free(cpu_mask_buf);
> +
> +       return cpu_map_addr;
> +}
> 
> The caller to this function expects the address of a cpumask_t
> data structure.  As I understand it, both before and after 2.6.29,
> the cpumask data structure looks like:
> 
>   typedef struct cpumask {
>       unsigned long int bits[<variable-based-upon-NR_CPUS>];
>   } cpumask_t;
> 
> And pre-2.6.29, the 3 "cpu_xxxx_map" symbols were all statically
> defined cpumask structures:
> 
>   cpumask_t cpu_online_mask;
>   cpumask_t cpu_possible_mask;
>   cpumask_t cpu_present_mask;
> 
> Whereas now the 3 symbols don't exist except as #define's like so:
> 
>   #define cpu_possible_map        (*(cpumask_t *)cpu_possible_mask)
>   #define cpu_online_map          (*(cpumask_t *)cpu_online_mask)
>   #define cpu_present_map         (*(cpumask_t *)cpu_present_mask)
> 
> and -- if I understand the code correctly -- each cpu_xxxx_mask 
> is just a pointer to a cpumask data structure.
> 
> So when the caller needs the address of the relevant cpumask data structure
> why doesn't your cpu_map_addr_from_mask() function simply do something
> like:
> 
>   get_symbol_data(cpu_mask_symbol, sizeof(void *), &tmp);
>   return tmp;
> 
> i.e., which would read the contents of the cpu_mask_symbol, which
> is the pointer that the caller wants.
>   
> Instead, your function reads in the whole data structure, where
> the cpumask.bits[] array typically contains several items, i.e.,
> enough longs to contain NR_CPUS bits.  Then, with ULONG(), it is
> essentially reading the unsigned long value found in the first
> bit[] array item, or:
> 
>    cpumask.bits[0];
> 
> and therefore returning the *bitmask* contained in the first unsigned
> long in the bit[] array -- instead of the *address* of the cpumask
> data structure.  How can that work? 
> 
> Or am I missing something?

No, you are completely right.

I tested my patch and it worked. But it only worked by chance. I read
the address using readmem(cpu_mask_size) in combination with ULONG(),
which is nonsense.

So the correct (and tested) version would be something like:

static ulong
get_cpu_map_addr_from_mask(const char *type)
{
        char cpu_mask_symbol[32];
        ulong cpu_map_addr;

        sprintf(cpu_mask_symbol, "cpu_%s_mask", type);

        if (!symbol_exists(cpu_mask_symbol))
                return 0;

        get_symbol_data(cpu_mask_symbol, sizeof(ulong), &cpu_map_addr);
        return cpu_map_addr;
}

Michael