Re: [Xen-ia64-devel] [Patch] iosapic virtualization again

[Tristan Gingold]

Le Mercredi 22 Février 2006 17:34, Tian, Kevin a écrit :
 Several coding style comments:

 1.
 + if (vector == IA64_TIMER_VECTOR || vector == IA64_IPI_VECTOR)
 + return 0;

 For this point, I second Eddie to have irq descriptor add a flag to
 indicate whether belonging to xen or guest, instead of hardcode here. You
 can grep IRQ_GUEST in xen/arch/x86. Though guest SMP and driver domain is
 not ready yet, interface should be designed clean and well to allow better
 cooperation between guest and xen.

 2. Not sure why you pull in iosapic.h into your patch. Seems no
 modification there which just need copy from linux source at compile time.
 If you really want to include this file, you can avoid adding
 xen_iosapic_write in c file and instead move its content to iosapic_write
 defined in iosapic.h
I suppose you speak about the iosapic.h for linux.
I have removed the iosapic_version() declaration, since it has been modified 
and is never used outside iosapic.

 3. Why ifdef XEN but nothing changed:
 +#ifdef XEN
 + vector = assign_irq_vector(AUTO_ASSIGN);
 +#else
 + /* If vector is running out, we try to find a sharable vector */
 +#endif
 + vector = assign_irq_vector(AUTO_ASSIGN);
Correct.

 4. It's ugly to see:
 +#define VCPU_XEN ((struct vcpu *)1)
 Also no place to init vcpu with this value, however later it's checked when
 reflecting interrupt
Maybe I should remove this ?

Tristan.


___
Xen-ia64-devel mailing list
Xen-ia64-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel

RE: [Xen-ia64-devel] [Patch] iosapic virtualization again

[Tian, Kevin]

From: Tristan Gingold [mailto:[EMAIL PROTECTED]
Sent: 2006年2月23日 16:15
 2. Not sure why you pull in iosapic.h into your patch. Seems no
 modification there which just need copy from linux source at compile time.
 If you really want to include this file, you can avoid adding
 xen_iosapic_write in c file and instead move its content to iosapic_write
 defined in iosapic.h
I suppose you speak about the iosapic.h for linux.

Yes.

I have removed the iosapic_version() declaration, since it has been modified
and is never used outside iosapic.

Then you should stick to CONFIG_XEN for future track of updates. Also
since this file is changed, I think you can remove xen_iosapic_write and 
instead extend iosapic_write in iosapic.h to have that check upon
running_on_xen. This can reduce modifications in iosapic.c to replace
iosapic_write with xen_iosapic_write. Same goes for read.


 4. It's ugly to see:
 +#define VCPU_XEN ((struct vcpu *)1)
 Also no place to init vcpu with this value, however later it's checked when
 reflecting interrupt
Maybe I should remove this ?


Yes. There's better way to indicate ownership of the specific vector.
Xen is started from Linux and then same concept is inherited that
so-called irq is the basic unit from system point of view. For 
small ia64 system, irq equals to vector but not true for large system. 
So it's natural to have guest indicator at irq level, like in irq_desc 
instead of at lower rte level, since multiple RTEs can be routed to 
same vector/irq.

You can see from linux history, interrupt sub-system evolves from
initial 80% (maybe inaccurate;-) arch dependent to nowadays 80%
arch independent. To this point, we should follow common model
defined in xen world.

See xen/arch/x86/irq.c, which shows you the picture what we suggest
to follow. ;-)

Thanks,
Kevin

___
Xen-ia64-devel mailing list
Xen-ia64-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel