Re: [RFC 1/2] Xen/ia64 modified files

[Alex Williamson <alex.williamson@hp.com>]

Hi Willy,

On Fri, 2006-06-02 at 14:24 -0600, Matthew Wilcox wrote:
> On Fri, Jun 02, 2006 at 01:09:14PM -0600, Alex Williamson wrote:
> > diff -r 016512c08f6b -r 4489a633a5de arch/ia64/Kconfig
> > --- a/arch/ia64/Kconfig	Thu May 25 03:00:07 2006 +0000
> > +++ b/arch/ia64/Kconfig	Fri Jun 02 09:54:29 2006 -0600
> > @@ -506,3 +520,5 @@ source "security/Kconfig"
> >  source "security/Kconfig"
> >  
> >  source "crypto/Kconfig"
> > +
> > +source "drivers/xen/Kconfig"
> 
> This should really be in drivers/Kconfig, though I suspect that's
> something for the xensource people.

   I agree, things like this should flow a little better when the xen
infrastructure is integrated upstream.


> >  drivers-$(CONFIG_PCI)		+= arch/ia64/pci/
> > +ifneq ($(CONFIG_XEN),y)
> >  drivers-$(CONFIG_IA64_HP_SIM)	+= arch/ia64/hp/sim/
> > +endif
> > +ifneq ($(CONFIG_IA64_GENERIC),y)
> > +drivers-$(CONFIG_XEN)		+= arch/ia64/hp/sim/
> > +endif
> 
> What're you trying to achieve here?  Why isn't it sufficient to add:
> 
> drivers-$(CONFIG_XEN) += arch/ia64/hp/sim/
> 
> and leave all the ifneqs out?

   Good question.  Some of this is probably kruft and I think we need to
look at whether all of the hpsim changes can simply be removed.  I'll
also mention that we're currently using only CONFIG_IA64_DIG for dom0
and domU kernels.  The zx1 and sn machvecs will need some porting to
understand the virtual physical memory idea (at least for their iommus).

> > @@ -70,6 +82,8 @@ all: compressed unwcheck
> >  all: compressed unwcheck
> >  
> >  compressed: vmlinux.gz
> > +
> > +vmlinuz: vmlinux.gz
> 
> This could be added independently

   Good idea, we should start cherry picking some of these for early
upstream inclusion.

> >  vmlinux.gz: vmlinux
> >  	$(Q)$(MAKE) $(build)=$(boot) $@
> > @@ -85,8 +99,8 @@ boot:	lib/lib.a vmlinux
> >  boot:	lib/lib.a vmlinux
> >  	$(Q)$(MAKE) $(build)=$(boot) $@
> >  
> > -install: vmlinux.gz
> > -	sh $(srctree)/arch/ia64/install.sh $(KERNELRELEASE) $< System.map "$(INSTALL_PATH)"
> > +install:
> > +	-yes | sh $(srctree)/arch/ia64/install.sh $(KERNELRELEASE) vmlinux.gz System.map "$(INSTALL_PATH)"
> 
> Huh?

   This is just to get around some build strangeness in the Xen tree,
probably not something that makes sense for the linux-ia64 tree.

> 
> I would suggest defining this in a header with the standard
> 
> #ifdef CONFIG_XEN
> #define is_running_on_xen() ...
> #else
> #define is_running_on_xen() 0
> #endif
> 
> trick.  This applies to just about every use of is_running_on_xen(),
> so I shan't comment on that further.

   Excellent cleanup idea.

> >  
> > +#ifdef CONFIG_XEN
> > +#ifndef CONFIG_IA64_HP_SIM
> > +	if (is_running_on_xen()) {
> > +		extern struct console hpsim_cons;
> > +		hpsim_cons.flags |= CON_BOOT;
> > +		register_console(&hpsim_cons);
> > +		earlycons++;
> > +	}
> > +#endif
> > +#endif
> 
> I'm not sure why you need the ifndef CONFIG_IA64_HP_SIM here?

   IMHO, this whole chunk is dated and probably needs to go.  This
provides something like an early_printk() through the hypervisor when
running on Xen.

> > diff -r 016512c08f6b -r 4489a633a5de arch/ia64/mm/ioremap.c
> > --- a/arch/ia64/mm/ioremap.c	Thu May 25 03:00:07 2006 +0000
> > +++ b/arch/ia64/mm/ioremap.c	Fri Jun 02 09:54:29 2006 -0600
> > @@ -15,6 +15,9 @@ static inline void __iomem *
> >  static inline void __iomem *
> >  __ioremap (unsigned long offset, unsigned long size)
> >  {
> > +#ifdef CONFIG_XEN
> > +	offset = HYPERVISOR_ioremap(offset, size);
> > +#endif
> >  	return (void __iomem *) (__IA64_UNCACHED_OFFSET | offset);
> >  }
> 
> I think this would be another great candidate for a dummy function.

   Yep, good idea.

> > -#define ia64_intrin_local_irq_restore(x)		\
> > +#define __ia64_intrin_local_irq_restore(x)		\
> 
> I presume with all these changes, the Xen header file does something
> like:
> 
> #ifdef CONFIG_XEN
> #define ia64_intrin_local_irq_restore(x) xen_ia64_intrin_local_irq_restore(x)
> #else
> #define ia64_intrin_local_irq_restore(x) __ia64_intrin_local_irq_restore(x)
> #endif

   Yes, where xen_ia64_intrin_local_irq_restore() supports native and
paravirtualized to allow a kernel with CONFIG_XEN to run on either.

> Overall, looks good.  I'll sit down at some point and really review the
> IRQ stuff, but this seems pretty clean to me.

   Great, thanks for jumping on this!  I'll see about incorporating your
suggestions into our tree.  Thanks,

	Alex

-- 
Alex Williamson                             HP Open Source & Linux Org.