RE: [Xen-devel] RE: [PATCH][DOM0] Expose physical CPU information in dom0

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

Konrad Rzeszutek Wilk wrote:
> Some little comments..
> 
> .. snip ..
> 
>> +    if (info->flags & XEN_PCPU_FLAGS_INVALID) {
>> +            /* The pcpu has been removed */
>> +            *result = 0;
> 
> You use #defines for the other cases. Should there be one for 0? Is 0
> PCPU_REMOVED? 

0 means no changes, < 0 means something wrong (like hypercall failure, or 
memory allocation failed).
The defined macro is for state changed (i.e. >0). Yes, we can define 0 as 
PCPU_NO_CHANGE.
For the code above, if the pcpu does not exist before in kernel, it is 0, means 
no change, otherwise it will be set as PCPU_REMOVED.

> 
>> +            if (pcpu) {
>> +                    raw_notifier_call_chain(&xen_pcpu_chain,
>> +                      XEN_PCPU_REMOVE,
>> +                      (void *)(long)pcpu->xen_id);
>> +                    xen_pcpu_free(pcpu);
>> +                    *result = PCPU_REMOVED;
>> +            }
>> +            return NULL;
>> +    }
>> +
>> +
>> +    if (!pcpu) {
>> +            *result = PCPU_ADDED;
>> +            pcpu = init_pcpu(info);
>> +            if (pcpu == NULL) {
>> +                    printk(KERN_WARNING "Failed to init pcpu %x\n", +       
>>                  
>> info->xen_cpuid); +                    *result = -1;
> 
> How about #define PCPU_BAD -1?

-1 means something wrong, not PCPU bad.

> 
> .. snip..
>> +/*
>> + * type: 0 add, 1 remove
>> + */
> 
> Why not make this an enum?
> 
>> +int xen_pcpu_hotplug(int type, uint32_t apic_id)

In fact, there is macro for it already. I give the value here to make it more 
clean.

    if (!found && (type == HOTPLUG_TYPE_ADD))
        printk(KERN_WARNING "The cpu is not added into Xen HV?\n");

    if (found && (type == HOTPLUG_TYPE_REMOVE))
        printk(KERN_WARNING "The cpu still exits in Xen HV?\n");
    return 0;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel