Re: [[RFC PATCH 2/8]: PVH: changes related to initial boot and irq rewiring

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Fri, 17 Aug 2012 09:56:20 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:

> On Thu, 2012-08-16 at 02:01 +0100, Mukesh Rathor wrote:
> [...]
> > @@ -1034,6 +1039,10 @@ static int xen_write_msr_safe(unsigned int
> > msr, unsigned low, unsigned high) 
> >  void xen_setup_shared_info(void)
> >  {
> > +	/* do later in xen_pvh_guest_init() when extend_brk is
> > properly setup*/
> > +	if (xen_pvh_domain() && xen_initial_domain())
> > +		return;
> 
> Could we push this setup later for a pv guest too and reduce the
> divergence?

A bit nervous changing PV paths until I've the bandwidth to test it
thoroughly with various mem configs. So, I'll put a TBD for now.

> > +
> >  	if (!xen_feature(XENFEAT_auto_translated_physmap)) {
> >  		set_fixmap(FIX_PARAVIRT_BOOTMAP,
> >  			   xen_start_info->shared_info);
> [...]
> > @@ -1274,6 +1287,10 @@ static const struct machine_ops
> > xen_machine_ops __initconst = { */
> >  static void __init xen_setup_stackprotector(void)
> >  {
> > +	if (xen_pvh_domain()) {
> > +		switch_to_new_gdt(0);
> 
> This seems to skip calling setup_stack_canary_segment too?
> 
> Assuming that's not deliberate I'd be tempted to just put "if
> (xen_pv_domain())" around the updates of pv_cpus_ops and leave the
> main flow of the code the same. If it was deliberate a comment might
> be in order.

I meant to comment to do this phase II. I'm not very familiar with
setup_stack_canary_segment stuff and will need to learn it first.
  

> >  }
> >  
> > +static void __init xen_pvh_guest_init(void)
> > +{
> > +#ifndef __HAVE_ARCH_PTE_SPECIAL
> > +	("__HAVE_ARCH_PTE_SPECIAL is required for PVH for now\n");
> > +	#error("__HAVE_ARCH_PTE_SPECIAL is required for PVH\n");
> > +#endif
> 
> Isn't this an unconditional feature of arch/x86?

Right. I can remove it now. I had started from much older linux.

> > diff --git a/arch/x86/xen/irq.c b/arch/x86/xen/irq.c
> > index 1573376..7c7dfd1 100644
> > --- a/arch/x86/xen/irq.c
> > +++ b/arch/x86/xen/irq.c
> > @@ -100,6 +100,10 @@ PV_CALLEE_SAVE_REGS_THUNK(xen_irq_enable);
> >  
> >  static void xen_safe_halt(void)
> >  {
> > +	/* so event channel can be delivered to us, since in HVM
> > container */
> > +	if (xen_pvh_domain())
> > +		local_irq_enable();
> > +
> >  	/* Blocking includes an implicit local_irq_enable(). */
> 
> So this comment isn't true for a PVH guest? Why not? Should it be?
 
I need to make sure the EFLAGS.IF is enabled. IIRC, the comment is saying
that xen will clear event channel mask bit. For PVH, there's the additional
EFLAGS.IF flag.