Linux Virtualization - Mar 2008 - [01/17]PATCH Add API for allocating dynamic TR resouce. V8

Hi Xiantao,

I general I think the code in this patch is fine. I have a couple of
nit-picking comments:
> +	if (target_mask&0x1) {
The formatting here isn't quite what most of the kernel does. It would
be better if you added spaces so it's a little easier to read, ie:

	if (target_mask & 0x1) {
> +		p = &__per_cpu_idtrs[cpu][0][0];
> +		for (i = IA64_TR_ALLOC_BASE; i <= per_cpu(ia64_tr_used,
> cpu);
> +								i++,
> p++) {
> +			if (p->pte&0x1)
Same thing here.
> +#define RR_TO_RID(rr) ((rr)<<32>>40)
I would prefer to have this one defined like this:

#define RR_TO_RID(rr)	(rr >> 8) & 0xffffff

It should generate the same code, but is more intuitive for the reader.

Otherwise I think this patch is fine - this is really just cosmetics.

Cheers,
Jes

Zhang, Xiantao wrote:
> +/*  mca_insert_tr
> + *
> + *  Switch rid when TR reload and needed!
> + *  iord: 1: itr, 2: itr;
> + *
> +*/
> +static void mca_insert_tr(u64 iord)
> +{
> +
> +	int i;
> +	u64 old_rr;
> +	struct ia64_tr_entry *p;
> +	unsigned long psr;
> +	int cpu = smp_processor_id();
What if CONFIG_PREEMPT is on, and we're being preempted and scheduled 
to a different CPU here? Are we running preempt disabled here? If so, 
the function header should state that this function needs to be called 
preempt_disabled.
> +/*
> + * ia64_insert_tr in virtual mode. Allocate a TR slot
> + *
> + * target_mask : 0x1 : itr, 0x2 : dtr, 0x3 : idtr
> + *
> + * va 	: virtual address.
> + * pte 	: pte entries inserted.
> + * log_size: range to be covered.
> + *
> + * Return value:  <0 :  error No.
> + *
> + *		  >=0 : slot number allocated for TR.
> + */
> +int ia64_itr_entry(u64 target_mask, u64 va, u64 pte, u64 log_size)
> +{
> +	int i, r;
> +	unsigned long psr;
> +	struct ia64_tr_entry *p;
> +	int cpu = smp_processor_id();
Same here.
> +/*
> + * ia64_purge_tr
> + *
> + * target_mask: 0x1: purge itr, 0x2 : purge dtr, 0x3 purge idtr.
> + *
> + * slot: slot number to be freed.
> + */
> +void ia64_ptr_entry(u64 target_mask, int slot)
> +{
> +	int cpu = smp_processor_id();
> +	int i;
> +	struct ia64_tr_entry *p;
Here again.

Zhang, Xiantao wrote:
> Carsten Otte wrote:
>> Zhang, Xiantao wrote:
>>> +/*  mca_insert_tr
>>> + *
>>> + *  Switch rid when TR reload and needed!
>>> + *  iord: 1: itr, 2: itr;
>>> + *
>>> +*/
>>> +static void mca_insert_tr(u64 iord)
>>> +{
>>> +
>>> +	int i;
>>> +	u64 old_rr;
>>> +	struct ia64_tr_entry *p;
>>> +	unsigned long psr;
>>> +	int cpu = smp_processor_id();
>> What if CONFIG_PREEMPT is on, and we're being preempted and
scheduled
>> to a different CPU here? Are we running preempt disabled here? If so,
>> the function header should state that this function needs to be called
>> preempt_disabled.
> 
> The function insert one TR to local TLB, and doesn't allow preempt
> before and after the call, so the caller should be with preempt_disable
> before calling into this routine. 
> Maybe the descripiton of this function should contain "Called with
> preempt disabled!".  Does it make sense ?
Yea, I think a comment would help in that case :-).