Xen devel - Aug 2012 - [RFC PATCH 1/8]: PVH: Basic and preparatory changes

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

It''s good to see well split patches :)
But could you please send them inline next time?

>  arch/x86/include/asm/xen/interface.h |    3 +-
>  arch/x86/include/asm/xen/page.h      |    3 ++
>  arch/x86/xen/setup.c                 |   13 ++++++++--
>  arch/x86/xen/smp.c                   |   39
++++++++++++++++++---------------
>  drivers/xen/cpu_hotplug.c            |    3 +-
>  include/xen/interface/xen.h          |    1 +
>  include/xen/xen.h                    |    4 +++
>  7 files changed, 43 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/include/asm/xen/interface.h
b/arch/x86/include/asm/xen/interface.h
> index cbf0c9d..1bd5e88 100644
> --- a/arch/x86/include/asm/xen/interface.h
> +++ b/arch/x86/include/asm/xen/interface.h
> @@ -136,7 +136,8 @@ struct vcpu_guest_context {
>      struct cpu_user_regs user_regs;         /* User-level CPU registers   
*/
>      struct trap_info trap_ctxt[256];        /* Virtual IDT                
*/
>      unsigned long ldt_base, ldt_ents;       /* LDT (linear address, #
ents) */
> -    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, #
ents) */
> +    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, #
ents).*
> +					     * PV in HVM: it''s GDTR addr/sz */
>      unsigned long kernel_ss, kernel_sp;     /* Virtual TSS (only SS1/SP1) 
*/
>      /* NB. User pagetable on x86/64 is placed in ctrlreg[1]. */
>      unsigned long ctrlreg[8];               /* CR0-CR7 (control registers)
*/
> diff --git a/arch/x86/include/asm/xen/page.h
b/arch/x86/include/asm/xen/page.h
> index 93971e8..d1cfb96 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -158,6 +158,9 @@ static inline xpaddr_t machine_to_phys(xmaddr_t
machine)
>  static inline unsigned long mfn_to_local_pfn(unsigned long mfn)
>  {
>  	unsigned long pfn = mfn_to_pfn(mfn);
> +
> +	if (xen_feature(XENFEAT_auto_translated_physmap))
> +		return mfn;
>  	if (get_phys_to_machine(pfn) != mfn)
>  		return -1; /* force !pfn_valid() */
>  	return pfn;
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index ead8557..936f21d 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -500,10 +500,9 @@ void __cpuinit xen_enable_syscall(void)
>  #endif /* CONFIG_X86_64 */
>  }
>  
> -void __init xen_arch_setup(void)
> +/* Normal PV domain not running in HVM container */
> +static __init void inline xen_non_pvh_arch_setup(void)
>  {
> -	xen_panic_handler_init();
> -
>  	HYPERVISOR_vm_assist(VMASST_CMD_enable, VMASST_TYPE_4gb_segments);
>  	HYPERVISOR_vm_assist(VMASST_CMD_enable, VMASST_TYPE_writable_pagetables);
>  
> @@ -517,6 +516,14 @@ void __init xen_arch_setup(void)
>  
>  	xen_enable_sysenter();
>  	xen_enable_syscall();
> +}
> +
> +void __init xen_arch_setup(void)
> +{
> +	xen_panic_handler_init();
> +
> +	if (!xen_pvh_domain())
> +		xen_non_pvh_arch_setup();
>  
>  #ifdef CONFIG_ACPI
>  	if (!(xen_start_info->flags & SIF_INITDOMAIN)) {
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index f58dca7..cdf269d 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -300,8 +300,6 @@ cpu_initialize_context(unsigned int cpu, struct
task_struct *idle)
>  	gdt = get_cpu_gdt_table(cpu);
>  
>  	ctxt->flags = VGCF_IN_KERNEL;
> -	ctxt->user_regs.ds = __USER_DS;
> -	ctxt->user_regs.es = __USER_DS;
>  	ctxt->user_regs.ss = __KERNEL_DS;
>  #ifdef CONFIG_X86_32
>  	ctxt->user_regs.fs = __KERNEL_PERCPU;
> @@ -314,31 +312,36 @@ cpu_initialize_context(unsigned int cpu, struct
task_struct *idle)
>  
>  	memset(&ctxt->fpu_ctxt, 0, sizeof(ctxt->fpu_ctxt));
>  
> -	xen_copy_trap_info(ctxt->trap_ctxt);
> +		ctxt->user_regs.ds = __USER_DS;
> +		ctxt->user_regs.es = __USER_DS;
>  
> -	ctxt->ldt_ents = 0;
> +		xen_copy_trap_info(ctxt->trap_ctxt);
>  
> -	BUG_ON((unsigned long)gdt & ~PAGE_MASK);
> +		ctxt->ldt_ents = 0;
>  
> -	gdt_mfn = arbitrary_virt_to_mfn(gdt);
> -	make_lowmem_page_readonly(gdt);
> -	make_lowmem_page_readonly(mfn_to_virt(gdt_mfn));
> +		BUG_ON((unsigned long)gdt & ~PAGE_MASK);
>  
> -	ctxt->gdt_frames[0] = gdt_mfn;
> -	ctxt->gdt_ents      = GDT_ENTRIES;
> +		gdt_mfn = arbitrary_virt_to_mfn(gdt);
> +		make_lowmem_page_readonly(gdt);
> +		make_lowmem_page_readonly(mfn_to_virt(gdt_mfn));
>  
> -	ctxt->user_regs.cs = __KERNEL_CS;
> -	ctxt->user_regs.esp = idle->thread.sp0 - sizeof(struct pt_regs);
> +		ctxt->gdt_frames[0] = gdt_mfn;
> +		ctxt->gdt_ents      = GDT_ENTRIES;
>  
> -	ctxt->kernel_ss = __KERNEL_DS;
> -	ctxt->kernel_sp = idle->thread.sp0;
> +		ctxt->kernel_ss = __KERNEL_DS;
> +		ctxt->kernel_sp = idle->thread.sp0;
>  
>  #ifdef CONFIG_X86_32
> -	ctxt->event_callback_cs     = __KERNEL_CS;
> -	ctxt->failsafe_callback_cs  = __KERNEL_CS;
> +		ctxt->event_callback_cs     = __KERNEL_CS;
> +		ctxt->failsafe_callback_cs  = __KERNEL_CS;
>  #endif
> -	ctxt->event_callback_eip    = (unsigned long)xen_hypervisor_callback;
> -	ctxt->failsafe_callback_eip = (unsigned long)xen_failsafe_callback;
> +		ctxt->event_callback_eip    > +					(unsigned
long)xen_hypervisor_callback;
> +		ctxt->failsafe_callback_eip > +					(unsigned
long)xen_failsafe_callback;
> +
> +	ctxt->user_regs.cs = __KERNEL_CS;
> +	ctxt->user_regs.esp = idle->thread.sp0 - sizeof(struct pt_regs);
>  
>  	per_cpu(xen_cr3, cpu) = __pa(swapper_pg_dir);
>  	ctxt->ctrlreg[3] = xen_pfn_to_cr3(virt_to_mfn(swapper_pg_dir));
> diff --git a/drivers/xen/cpu_hotplug.c b/drivers/xen/cpu_hotplug.c
> index 4dcfced..a797359 100644
> --- a/drivers/xen/cpu_hotplug.c
> +++ b/drivers/xen/cpu_hotplug.c
> @@ -100,7 +100,8 @@ static int __init setup_vcpu_hotplug_event(void)
>  	static struct notifier_block xsn_cpu = {
>  		.notifier_call = setup_cpu_watcher };
>  
> -	if (!xen_pv_domain())
> +	/* PVH TBD/FIXME: future work */
> +	if (!xen_pv_domain() || xen_pvh_domain())
>  		return -ENODEV;
>  
>  	register_xenstore_notifier(&xsn_cpu);
I don''t think we should use or have xen_pvh_domain() in non-x86 files,
like anything under drivers/xen.
Isn''t XENFEAT_auto_translated_physmap enough?

> diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
> index 0801468..1d5bc36 100644
> --- a/include/xen/interface/xen.h
> +++ b/include/xen/interface/xen.h
> @@ -493,6 +493,7 @@ struct dom0_vga_console_info {
>  /* These flags are passed in the ''flags'' field of
start_info_t. */
>  #define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged? */
>  #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial control
domain? */
> +#define SIF_IS_PVINHVM    (1<<4)  /* Is it a PV running in HVM
container? */
>  #define SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for xen-pm
options */
>  
>  typedef uint64_t cpumap_t;
I would avoid adding SIF_IS_PVINHVM, an x86 specific concept, into a
generic xen.h interface file. 

> diff --git a/include/xen/xen.h b/include/xen/xen.h
> index a164024..e823639 100644
> --- a/include/xen/xen.h
> +++ b/include/xen/xen.h
> @@ -18,6 +18,10 @@ extern enum xen_domain_type xen_domain_type;
>  				 xen_domain_type == XEN_PV_DOMAIN)
>  #define xen_hvm_domain()	(xen_domain() &&			\
>  				 xen_domain_type == XEN_HVM_DOMAIN)
> +/* xen_pv_domain check is necessary as start_info ptr is null in HVM.
Also,
> + * note, xen PVH domain shares lot of HVM code */
> +#define xen_pvh_domain()       (xen_pv_domain() &&                
\
> +				(xen_start_info->flags & SIF_IS_PVINHVM))
 
Also here.

On Thu, 16 Aug 2012 14:59:14 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> It''s good to see well split patches :)
> But could you please send them inline next time?
Sorry, that was the intent. 
> >  
> > -	if (!xen_pv_domain())
> > +	/* PVH TBD/FIXME: future work */
> > +	if (!xen_pv_domain() || xen_pvh_domain())
> >  		return -ENODEV;
> >  
> >  	register_xenstore_notifier(&xsn_cpu);
> 
> I don''t think we should use or have xen_pvh_domain() in non-x86
files,
> like anything under drivers/xen.
> Isn''t XENFEAT_auto_translated_physmap enough?
Ah, ok, I''ll change it.
> > diff --git a/include/xen/interface/xen.h
> > b/include/xen/interface/xen.h index 0801468..1d5bc36 100644
> > --- a/include/xen/interface/xen.h
> > +++ b/include/xen/interface/xen.h
> > @@ -493,6 +493,7 @@ struct dom0_vga_console_info {
> >  /* These flags are passed in the ''flags'' field of
start_info_t. */
> >  #define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged?
*/
> >  #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial
control
> > domain? */ +#define SIF_IS_PVINHVM    (1<<4)  /* Is it a PV
running
> > in HVM container? */ #define SIF_PM_MASK       (0xFF<<8) /*
reserve
> > 1 byte for xen-pm options */ 
> >  typedef uint64_t cpumap_t;
> 
> I would avoid adding SIF_IS_PVINHVM, an x86 specific concept, into a
> generic xen.h interface file. 
> > +/* xen_pv_domain check is necessary as start_info ptr is null in
> > HVM. Also,
> > + * note, xen PVH domain shares lot of HVM code */
> > +#define xen_pvh_domain()       (xen_pv_domain()
> > &&                     \
> > +				(xen_start_info->flags &
> > SIF_IS_PVINHVM))
>  
> Also here.
Hmm.. I can move ''#define xen_pvh_domain()'' to x86 header,
easy. But,
not sure how to define SIF_IS_PVINHVM then? I could put SIF_IS_RESVD in
include/xen/interface/xen.h, and then do 
#define SIF_IS_PVINHVM SIF_IS_RESVD in an x86 file.

What do you think about that?

thanks
Mukesh

On Thu, 2012-08-16 at 19:46 +0100, Mukesh Rathor wrote:
> On Thu, 16 Aug 2012 14:59:14 +0100
> Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> 
> > It''s good to see well split patches :)
> > But could you please send them inline next time?
> 
> Sorry, that was the intent. 
It seems like this was the only one which was attached instead of
inline, strange.
> > > diff --git a/include/xen/interface/xen.h
> > > b/include/xen/interface/xen.h index 0801468..1d5bc36 100644
> > > --- a/include/xen/interface/xen.h
> > > +++ b/include/xen/interface/xen.h
> > > @@ -493,6 +493,7 @@ struct dom0_vga_console_info {
> > >  /* These flags are passed in the ''flags'' field
of start_info_t. */
> > >  #define SIF_PRIVILEGED    (1<<0)  /* Is the domain
privileged? */
> > >  #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial
control
> > > domain? */ +#define SIF_IS_PVINHVM    (1<<4)  /* Is it a PV
running
> > > in HVM container? */ #define SIF_PM_MASK       (0xFF<<8) /*
reserve
> > > 1 byte for xen-pm options */ 
> > >  typedef uint64_t cpumap_t;
> > 
> > I would avoid adding SIF_IS_PVINHVM, an x86 specific concept, into a
> > generic xen.h interface file. 
Is PVH actually more like a XENFEAT style thing?

Is there actually anywhere which wants to know specifically about PVH
rather than some more specific property which a PVH domain happen to
has?
> > > +/* xen_pv_domain check is necessary as start_info ptr is null in
> > > HVM. Also,
> > > + * note, xen PVH domain shares lot of HVM code */
> > > +#define xen_pvh_domain()       (xen_pv_domain()
> > > &&                     \
> > > +				(xen_start_info->flags &
> > > SIF_IS_PVINHVM))
> >  
> > Also here.
> 
> Hmm.. I can move ''#define xen_pvh_domain()'' to x86
header, easy. But,
> not sure how to define SIF_IS_PVINHVM then? I could put SIF_IS_RESVD in
> include/xen/interface/xen.h, and then do 
> #define SIF_IS_PVINHVM SIF_IS_RESVD in an x86 file.
> 
> What do you think about that?
Should PVH actually be a new value in the xen_domain_type enum?
> 
> thanks
> Mukesh
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

On Thu, 2012-08-16 at 01:57 +0100, Mukesh Rathor wrote:
> ---
>  arch/x86/include/asm/xen/interface.h |    3 +-
>  arch/x86/include/asm/xen/page.h      |    3 ++
>  arch/x86/xen/setup.c                 |   13 ++++++++--
>  arch/x86/xen/smp.c                   |   39
++++++++++++++++++---------------
>  drivers/xen/cpu_hotplug.c            |    3 +-
>  include/xen/interface/xen.h          |    1 +
>  include/xen/xen.h                    |    4 +++
>  7 files changed, 43 insertions(+), 23 deletions(-)
> 
> diff --git a/arch/x86/include/asm/xen/interface.h
b/arch/x86/include/asm/xen/interface.h
> index cbf0c9d..1bd5e88 100644
> --- a/arch/x86/include/asm/xen/interface.h
> +++ b/arch/x86/include/asm/xen/interface.h
> @@ -136,7 +136,8 @@ struct vcpu_guest_context {
>      struct cpu_user_regs user_regs;         /* User-level CPU registers   
*/
>      struct trap_info trap_ctxt[256];        /* Virtual IDT                
*/
>      unsigned long ldt_base, ldt_ents;       /* LDT (linear address, #
ents) */
> -    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, #
ents) */
> +    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine frames, #
ents).*
> +                                            * PV in HVM: it''s
GDTR addr/sz */
I''m not sure I understand this comment. What is "GDTR
addr/sz" do you
mean that gdtframes/gdt_ents has a different semantics here?

Might be worthy of a union? Or finding some other way to expand this struct.
> 
>      unsigned long kernel_ss, kernel_sp;     /* Virtual TSS (only SS1/SP1) 
*/
>      /* NB. User pagetable on x86/64 is placed in ctrlreg[1]. */
>      unsigned long ctrlreg[8];               /* CR0-CR7 (control registers)
*/
> 
> diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c
> index ead8557..936f21d 100644
> --- a/arch/x86/xen/setup.c
> +++ b/arch/x86/xen/setup.c
> @@ -500,10 +500,9 @@ void __cpuinit xen_enable_syscall(void)
>  #endif /* CONFIG_X86_64 */
>  }
>  
> -void __init xen_arch_setup(void)
> +/* Normal PV domain not running in HVM container */
It''s a bit of a shame to overload the "HVM" term this way, to
mean both
the traditional "providing a full PC like environment" and "PV
using
hardware virtualisation facilities".

Perhaps:
        /* Normal PV domain without PVH extensions */
> +static __init void inline xen_non_pvh_arch_setup(void)
>  {
> -       xen_panic_handler_init();
> -
>         HYPERVISOR_vm_assist(VMASST_CMD_enable, VMASST_TYPE_4gb_segments);
>         HYPERVISOR_vm_assist(VMASST_CMD_enable,
VMASST_TYPE_writable_pagetables);
>  
> @@ -517,6 +516,14 @@ void __init xen_arch_setup(void)
>  
>         xen_enable_sysenter();
>         xen_enable_syscall();
> +}
> +
> +void __init xen_arch_setup(void)
> +{
> +       xen_panic_handler_init();
> +
> +       if (!xen_pvh_domain())
> +               xen_non_pvh_arch_setup();
The negative in the fn name here strikes me as a bit weird. Can''t this
just be xen_pv_arch_setup?

Or even just have:
	/* Everything else is specific to PV without hardware support */
	if (xen_pvh_domain())
		return;

>  
>  #ifdef CONFIG_ACPI
>         if (!(xen_start_info->flags & SIF_INITDOMAIN)) {
> diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> index f58dca7..cdf269d 100644
> --- a/arch/x86/xen/smp.c
> +++ b/arch/x86/xen/smp.c
> @@ -300,8 +300,6 @@ cpu_initialize_context(unsigned int cpu, struct
task_struct *idle)
>         gdt = get_cpu_gdt_table(cpu);
>  
>         ctxt->flags = VGCF_IN_KERNEL;
> -       ctxt->user_regs.ds = __USER_DS;
> -       ctxt->user_regs.es = __USER_DS;
>         ctxt->user_regs.ss = __KERNEL_DS;
>  #ifdef CONFIG_X86_32
>         ctxt->user_regs.fs = __KERNEL_PERCPU;
> @@ -314,31 +312,36 @@ cpu_initialize_context(unsigned int cpu, struct
task_struct *idle)
>  
>         memset(&ctxt->fpu_ctxt, 0, sizeof(ctxt->fpu_ctxt));
>  
> -       xen_copy_trap_info(ctxt->trap_ctxt);
> +               ctxt->user_regs.ds = __USER_DS;
> +               ctxt->user_regs.es = __USER_DS;
>  
> -       ctxt->ldt_ents = 0;
> +               xen_copy_trap_info(ctxt->trap_ctxt);
>  
> -       BUG_ON((unsigned long)gdt & ~PAGE_MASK);
> +               ctxt->ldt_ents = 0;
Something odd is going on with the indentation here (and below I''ve
just
noticed). I suspect lots of the changes aren''t really changing anything
other than whitespace?
> -       gdt_mfn = arbitrary_virt_to_mfn(gdt);
> -       make_lowmem_page_readonly(gdt);
> -       make_lowmem_page_readonly(mfn_to_virt(gdt_mfn));
> +               BUG_ON((unsigned long)gdt & ~PAGE_MASK);
>  
> -       ctxt->gdt_frames[0] = gdt_mfn;
> -       ctxt->gdt_ents      = GDT_ENTRIES;
> +               gdt_mfn = arbitrary_virt_to_mfn(gdt);
> +               make_lowmem_page_readonly(gdt);
> +               make_lowmem_page_readonly(mfn_to_virt(gdt_mfn));
>  
> -       ctxt->user_regs.cs = __KERNEL_CS;
> -       ctxt->user_regs.esp = idle->thread.sp0 - sizeof(struct
pt_regs);
> +               ctxt->gdt_frames[0] = gdt_mfn;
> +               ctxt->gdt_ents      = GDT_ENTRIES;
>  
> -       ctxt->kernel_ss = __KERNEL_DS;
> -       ctxt->kernel_sp = idle->thread.sp0;
> +               ctxt->kernel_ss = __KERNEL_DS;
> +               ctxt->kernel_sp = idle->thread.sp0;
>  
>  #ifdef CONFIG_X86_32
> -       ctxt->event_callback_cs     = __KERNEL_CS;
> -       ctxt->failsafe_callback_cs  = __KERNEL_CS;
> +               ctxt->event_callback_cs     = __KERNEL_CS;
> +               ctxt->failsafe_callback_cs  = __KERNEL_CS;
>  #endif
> -       ctxt->event_callback_eip    = (unsigned
long)xen_hypervisor_callback;
> -       ctxt->failsafe_callback_eip = (unsigned
long)xen_failsafe_callback;
> +               ctxt->event_callback_eip    > +                      
(unsigned long)xen_hypervisor_callback;
> +               ctxt->failsafe_callback_eip > +                      
(unsigned long)xen_failsafe_callback;
> +
> +       ctxt->user_regs.cs = __KERNEL_CS;
> +       ctxt->user_regs.esp = idle->thread.sp0 - sizeof(struct
pt_regs);
>  
>         per_cpu(xen_cr3, cpu) = __pa(swapper_pg_dir);
>         ctxt->ctrlreg[3] = xen_pfn_to_cr3(virt_to_mfn(swapper_pg_dir));
Ian.

On Thu, 16 Aug 2012, Mukesh Rathor wrote:
> > > diff --git a/include/xen/interface/xen.h
> > > b/include/xen/interface/xen.h index 0801468..1d5bc36 100644
> > > --- a/include/xen/interface/xen.h
> > > +++ b/include/xen/interface/xen.h
> > > @@ -493,6 +493,7 @@ struct dom0_vga_console_info {
> > >  /* These flags are passed in the ''flags'' field
of start_info_t. */
> > >  #define SIF_PRIVILEGED    (1<<0)  /* Is the domain
privileged? */
> > >  #define SIF_INITDOMAIN    (1<<1)  /* Is this the initial
control
> > > domain? */ +#define SIF_IS_PVINHVM    (1<<4)  /* Is it a PV
running
> > > in HVM container? */ #define SIF_PM_MASK       (0xFF<<8) /*
reserve
> > > 1 byte for xen-pm options */ 
> > >  typedef uint64_t cpumap_t;
> > 
> > I would avoid adding SIF_IS_PVINHVM, an x86 specific concept, into a
> > generic xen.h interface file. 
> 
> > > +/* xen_pv_domain check is necessary as start_info ptr is null in
> > > HVM. Also,
> > > + * note, xen PVH domain shares lot of HVM code */
> > > +#define xen_pvh_domain()       (xen_pv_domain()
> > > &&                     \
> > > +				(xen_start_info->flags &
> > > SIF_IS_PVINHVM))
> >  
> > Also here.
> 
> Hmm.. I can move ''#define xen_pvh_domain()'' to x86
header, easy. But,
> not sure how to define SIF_IS_PVINHVM then? I could put SIF_IS_RESVD in
> include/xen/interface/xen.h, and then do 
> #define SIF_IS_PVINHVM SIF_IS_RESVD in an x86 file.
> 
> What do you think about that?
I am not particularly fussed about the location of SIF_IS_PVINHVM.
We could define it in asm/xen/hypervisor.h for example.

The very important bit is to avoid xen_pvh_domain() in generic code
because it reduces code reusability.
xen_pvh_domain() covers too many different concepts (a PV guest, in an
HVM container, using nested paging in hardware), if we bundle them
together it makes it much harder to reuse them.
So we should avoid checking for xen_pvh_domain() and check for the
relevant sub-property we actually interested in.

For example in balloon.c we are probably only interested in memory
related behavior, so checking for XENFEAT_auto_translated_physmap should
be enough.  In other parts of the code we might want to check for
xen_pv_domain(). If xen_pv_domain() and XENFEAT_auto_translated_physmap
are not enough, we could introduce another small XENFEAT that specifies
that the domain is running in a HVM container. This way they are all
reusable.

On Fri, 17 Aug 2012, Ian Campbell wrote:
> > > > diff --git a/include/xen/interface/xen.h
> > > > b/include/xen/interface/xen.h index 0801468..1d5bc36 100644
> > > > --- a/include/xen/interface/xen.h
> > > > +++ b/include/xen/interface/xen.h
> > > > @@ -493,6 +493,7 @@ struct dom0_vga_console_info {
> > > >  /* These flags are passed in the ''flags''
field of start_info_t. */
> > > >  #define SIF_PRIVILEGED    (1<<0)  /* Is the domain
privileged? */
> > > >  #define SIF_INITDOMAIN    (1<<1)  /* Is this the
initial control
> > > > domain? */ +#define SIF_IS_PVINHVM    (1<<4)  /* Is it
a PV running
> > > > in HVM container? */ #define SIF_PM_MASK      
(0xFF<<8) /* reserve
> > > > 1 byte for xen-pm options */ 
> > > >  typedef uint64_t cpumap_t;
> > > 
> > > I would avoid adding SIF_IS_PVINHVM, an x86 specific concept,
into a
> > > generic xen.h interface file. 
> 
> Is PVH actually more like a XENFEAT style thing?
> 
> Is there actually anywhere which wants to know specifically about PVH
> rather than some more specific property which a PVH domain happen to
> has?
That''s exactly the point.

> > > > +/* xen_pv_domain check is necessary as start_info ptr is
null in
> > > > HVM. Also,
> > > > + * note, xen PVH domain shares lot of HVM code */
> > > > +#define xen_pvh_domain()       (xen_pv_domain()
> > > > &&                     \
> > > > +				(xen_start_info->flags &
> > > > SIF_IS_PVINHVM))
> > >  
> > > Also here.
> > 
> > Hmm.. I can move ''#define xen_pvh_domain()'' to x86
header, easy. But,
> > not sure how to define SIF_IS_PVINHVM then? I could put SIF_IS_RESVD
in
> > include/xen/interface/xen.h, and then do 
> > #define SIF_IS_PVINHVM SIF_IS_RESVD in an x86 file.
> > 
> > What do you think about that?
> 
> Should PVH actually be a new value in the xen_domain_type enum?
I don''t think we should have a xen_domain_type pvh at all.
If we really need it we should define it as a set of individual
properties:

#define xen_pvh_domain() (xen_pv_domain() && \
                          xen_feature(XENFEAT_auto_translated_physmap)
&& \
                          xen_have_vector_callback)

On Fri, 2012-08-17 at 11:56 +0100, Stefano Stabellini
wrote:
> On Fri, 17 Aug 2012, Ian Campbell wrote:
> > > > > +/* xen_pv_domain check is necessary as start_info ptr
is null in
> > > > > HVM. Also,
> > > > > + * note, xen PVH domain shares lot of HVM code */
> > > > > +#define xen_pvh_domain()       (xen_pv_domain()
> > > > > &&                     \
> > > > > +				(xen_start_info->flags &
> > > > > SIF_IS_PVINHVM))
> > > >  
> > > > Also here.
> > > 
> > > Hmm.. I can move ''#define xen_pvh_domain()'' to
x86 header, easy. But,
> > > not sure how to define SIF_IS_PVINHVM then? I could put
SIF_IS_RESVD in
> > > include/xen/interface/xen.h, and then do 
> > > #define SIF_IS_PVINHVM SIF_IS_RESVD in an x86 file.
> > > 
> > > What do you think about that?
> > 
> > Should PVH actually be a new value in the xen_domain_type enum?
> 
> I don''t think we should have a xen_domain_type pvh at all.
> If we really need it we should define it as a set of individual
> properties:
> 
> #define xen_pvh_domain() (xen_pv_domain() && \
>                           xen_feature(XENFEAT_auto_translated_physmap)
&& \
>                           xen_have_vector_callback)
Hopefully it won''t be necessary but if it is then this seems like a
good
idea to me.

Ian.

On Fri, 17 Aug 2012 11:56:43 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 17 Aug 2012, Ian Campbell wrote:
> > > > > diff --git a/include/xen/interface/xen.h
> > > > > b/include/xen/interface/xen.h index 0801468..1d5bc36
100644
> > > > > --- a/include/xen/interface/xen.h
> > > > > +++ b/include/xen/interface/xen.h
> > > > > @@ -493,6 +493,7 @@ struct dom0_vga_console_info {
> > > > >  /* These flags are passed in the
''flags'' field of
> > > > > start_info_t. */ #define SIF_PRIVILEGED    (1<<0)
/* Is the
> > > > > domain privileged? */ #define SIF_INITDOMAIN   
(1<<1)  /* Is
> > > > > this the initial control domain? */ +#define
> > > > > SIF_IS_PVINHVM    (1<<4)  /* Is it a PV running
in HVM
> > > > > container? */ #define SIF_PM_MASK       (0xFF<<8)
/* reserve
> > > > > 1 byte for xen-pm options */ typedef uint64_t cpumap_t;
> > > > 
> > > > I would avoid adding SIF_IS_PVINHVM, an x86 specific
concept,
> > > > into a generic xen.h interface file. 
> > 
> > Is PVH actually more like a XENFEAT style thing?
> > 
> > Is there actually anywhere which wants to know specifically about
> > PVH rather than some more specific property which a PVH domain
> > happen to has?
> 
> That''s exactly the point.
> 
> 
> > > > > +/* xen_pv_domain check is necessary as start_info ptr
is
> > > > > null in HVM. Also,
> > > > > + * note, xen PVH domain shares lot of HVM code */
> > > > > +#define xen_pvh_domain()       (xen_pv_domain()
> > > > > &&                     \
> > > > > +				(xen_start_info->flags &
> > > > > SIF_IS_PVINHVM))
> > > >  
> > > > Also here.
> > > 
> > > Hmm.. I can move ''#define xen_pvh_domain()'' to
x86 header, easy.
> > > But, not sure how to define SIF_IS_PVINHVM then? I could put
> > > SIF_IS_RESVD in include/xen/interface/xen.h, and then do 
> > > #define SIF_IS_PVINHVM SIF_IS_RESVD in an x86 file.
> > > 
> > > What do you think about that?
> > 
> > Should PVH actually be a new value in the xen_domain_type enum?
> 
> I don''t think we should have a xen_domain_type pvh at all.
> If we really need it we should define it as a set of individual
> properties:
> 
> #define xen_pvh_domain() (xen_pv_domain() && \
>                           xen_feature(XENFEAT_auto_translated_physmap)
> && \ xen_have_vector_callback)
No, I had started with enum, too many code unnecessary code changes
then. I like the above #define. It eradicates the need for SIF flag.
I''ll see if that works.

On Fri, 17 Aug 2012 11:15:32 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 16 Aug 2012, Mukesh Rathor wrote:
> > > > diff --git a/include/xen/interface/xen.h
> > > > b/include/xen/interface/xen.h index 0801468..1d5bc36 100644
> > > > --- a/include/xen/interface/xen.h
> > > > +++ b/include/xen/interface/xen.h
> > > > @@ -493,6 +493,7 @@ struct dom0_vga_console_info {
> > > >  /* These flags are passed in the ''flags''
field of
> > > > start_info_t. */ #define SIF_PRIVILEGED    (1<<0)  /*
Is the
> > > > domain privileged? */ #define SIF_INITDOMAIN    (1<<1)
/* Is
> > > > this the initial control domain? */ +#define SIF_IS_PVINHVM
> > > > (1<<4)  /* Is it a PV running in HVM container? */
#define
> > > > SIF_PM_MASK       (0xFF<<8) /* reserve 1 byte for
xen-pm
> > > > options */ typedef uint64_t cpumap_t;
> > > 
> > > I would avoid adding SIF_IS_PVINHVM, an x86 specific concept,
> > > into a generic xen.h interface file. 
> > 
> > > > +/* xen_pv_domain check is necessary as start_info ptr is
null
> > > > in HVM. Also,
> > > > + * note, xen PVH domain shares lot of HVM code */
> > > > +#define xen_pvh_domain()       (xen_pv_domain()
> > > > &&                     \
> > > > +				(xen_start_info->flags &
> > > > SIF_IS_PVINHVM))
> > >  
> > > Also here.
> > 
> > Hmm.. I can move ''#define xen_pvh_domain()'' to x86
header, easy.
> > But, not sure how to define SIF_IS_PVINHVM then? I could put
> > SIF_IS_RESVD in include/xen/interface/xen.h, and then do 
> > #define SIF_IS_PVINHVM SIF_IS_RESVD in an x86 file.
> > 
> > What do you think about that?
> 
> I am not particularly fussed about the location of SIF_IS_PVINHVM.
> We could define it in asm/xen/hypervisor.h for example.
> 
> The very important bit is to avoid xen_pvh_domain() in generic code
> because it reduces code reusability.
> xen_pvh_domain() covers too many different concepts (a PV guest, in an
> HVM container, using nested paging in hardware), if we bundle them
> together it makes it much harder to reuse them.
> So we should avoid checking for xen_pvh_domain() and check for the
> relevant sub-property we actually interested in.
> 
> For example in balloon.c we are probably only interested in memory
> related behavior, so checking for XENFEAT_auto_translated_physmap
> should be enough.  In other parts of the code we might want to check
> for xen_pv_domain(). If xen_pv_domain() and
> XENFEAT_auto_translated_physmap are not enough, we could introduce
> another small XENFEAT that specifies that the domain is running in a
> HVM container. This way they are all reusable.
yeah, I thought about that, but wasn''t sure what the implications would
be for a guest thats not PVH but has auto xlated physmap, if there''s
such a possibility. If you guys think thats not an issue, I can change
it.

On Fri, 17 Aug 2012 09:35:54 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2012-08-16 at 01:57 +0100, Mukesh Rathor wrote:
> 
> > LDT (linear address, # ents) */
> > -    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine
> > frames, # ents) */
> > +    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine
> > frames, # ents).*
> > +                                            * PV in HVM:
it''s GDTR
> > addr/sz */
> 
> I''m not sure I understand this comment. What is "GDTR
addr/sz" do you
> mean that gdtframes/gdt_ents has a different semantics here?
> 
> Might be worthy of a union? Or finding some other way to expand this
> struct.
In case of PVH, the field is used to send down GDTR address and size.
perhaps better to just leave the comment out.
> > 
> > -void __init xen_arch_setup(void)
> > +/* Normal PV domain not running in HVM container */
> 
> It''s a bit of a shame to overload the "HVM" term this
way, to mean
> both the traditional "providing a full PC like environment" and
"PV
> using hardware virtualisation facilities".
> 
> Perhaps:
>         /* Normal PV domain without PVH extensions */
Ok, HVM==Hardware Virtual Machine seems more appropriate here, but I
can remove the word HVM and go with ''PVH extensions''.

> > +static __init void inline xen_non_pvh_arch_setup(void)
> > +       xen_panic_handler_init();
> > +
> > +       if (!xen_pvh_domain())
> > +               xen_non_pvh_arch_setup();
> 
> The negative in the fn name here strikes me as a bit weird. Can''t
this
> just be xen_pv_arch_setup?
> 
> Or even just have:
> 	/* Everything else is specific to PV without hardware support
> */ if (xen_pvh_domain())
> 		return;
OK.
> >  
> >  #ifdef CONFIG_ACPI
> >         if (!(xen_start_info->flags & SIF_INITDOMAIN)) {
> > diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> > index f58dca7..cdf269d 100644
> > --- a/arch/x86/xen/smp.c
> > +++ b/arch/x86/xen/smp.c
> > @@ -300,8 +300,6 @@ cpu_initialize_context(unsigned int cpu, struct
> > task_struct *idle) gdt = get_cpu_gdt_table(cpu);
> > -       BUG_ON((unsigned long)gdt & ~PAGE_MASK);
> > +               ctxt->ldt_ents = 0;
> 
> Something odd is going on with the indentation here (and below
I''ve
> just noticed). I suspect lots of the changes aren''t really
changing
> anything other than whitespace?
Konrad wanted just doing the indentation without code change first
where it made sense so that further patch makes it easy to see if
statements added.

On Fri, 2012-08-17 at 20:20 +0100, Mukesh Rathor wrote:
> yeah, I thought about that, but wasn''t sure what the implications
would
> be for a guest thats not PVH but has auto xlated physmap, if
there''s
> such a possibility. If you guys think thats not an issue, I can change
> it.
I think you are basically never going to come across that situation in
real life. But if you are worried you could check for that combination
at boot and panic or something like that, which would mean you could not
worry about it at runtime.

Ian.

> > For example in balloon.c we are probably only interested in memory
> > related behavior, so checking for XENFEAT_auto_translated_physmap
> > should be enough.  In other parts of the code we might want to check
> > for xen_pv_domain(). If xen_pv_domain() and
> > XENFEAT_auto_translated_physmap are not enough, we could introduce
> > another small XENFEAT that specifies that the domain is running in a
> > HVM container. This way they are all reusable.
> 
> yeah, I thought about that, but wasn''t sure what the implications
would
> be for a guest thats not PVH but has auto xlated physmap, if
there''s
> such a possibility. If you guys think thats not an issue, I can change
> it.
dom0_shadow=on on the hypervisor mode enables that in PV mode.

On Fri, 2012-08-17 at 20:24 +0100, Mukesh Rathor wrote:
> On Fri, 17 Aug 2012 09:35:54 +0100
> Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> > On Thu, 2012-08-16 at 01:57 +0100, Mukesh Rathor wrote:
> > 
> > > LDT (linear address, # ents) */
> > > -    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine
> > > frames, # ents) */
> > > +    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine
> > > frames, # ents).*
> > > +                                            * PV in HVM:
it''s GDTR
> > > addr/sz */
> > 
> > I''m not sure I understand this comment. What is "GDTR
addr/sz" do you
> > mean that gdtframes/gdt_ents has a different semantics here?
> > 
> > Might be worthy of a union? Or finding some other way to expand this
> > struct.
> 
> In case of PVH, the field is used to send down GDTR address and size.
> perhaps better to just leave the comment out.
I think if the semantics of this field are totally different in the two
modes then I think a union is warranted.
> > > -void __init xen_arch_setup(void)
> > > +/* Normal PV domain not running in HVM container */
> > 
> > It''s a bit of a shame to overload the "HVM" term
this way, to mean
> > both the traditional "providing a full PC like environment"
and "PV
> > using hardware virtualisation facilities".
> > 
> > Perhaps:
> >         /* Normal PV domain without PVH extensions */
> 
> Ok, HVM==Hardware Virtual Machine seems more appropriate here,
HVM in the context of Xen means more than that though, it implies a Qemu
and emulation of a complete "PC-like" environment and all of that
stuff,
which is why I think it is inappropriate/confusing to be overloading it
to mean "PV with hardware assistance" too.

Xen''s use of the term HVM is a bit unhelpful, exactly because it is a
broad sounding term but with a very specific meaning, but we are kind of
stuck with it.
> but I can remove the word HVM and go with ''PVH
extensions''.
Please ;-)
> 
> > >  
> > >  #ifdef CONFIG_ACPI
> > >         if (!(xen_start_info->flags & SIF_INITDOMAIN)) {
> > > diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> > > index f58dca7..cdf269d 100644
> > > --- a/arch/x86/xen/smp.c
> > > +++ b/arch/x86/xen/smp.c
> > > @@ -300,8 +300,6 @@ cpu_initialize_context(unsigned int cpu,
struct
> > > task_struct *idle) gdt = get_cpu_gdt_table(cpu);
> > > -       BUG_ON((unsigned long)gdt & ~PAGE_MASK);
> > > +               ctxt->ldt_ents = 0;
> > 
> > Something odd is going on with the indentation here (and below
I''ve
> > just noticed). I suspect lots of the changes aren''t really
changing
> > anything other than whitespace?
> 
> Konrad wanted just doing the indentation without code change first
> where it made sense so that further patch makes it easy to see if
> statements added.
I disagree that this is a useful way to structure a series unless the
whitespace change is in a patch of *only* whitespace changes (and even
then this would be an uncommon way to do things IMO).

But putting the whitespace changes associated with adding an if
alongside unrelated actual semantic changes in a totally different patch
is probably the most confusing and least helpful of all the possible
options!

My personal preference would be to do the indent when adding the if and
let people who want to see the differences without the indentation
change use "diff -b".

Konrad is maintainer though, so if he likes this then fine.

Ian.

On Fri, Aug 17, 2012 at 08:47:02PM +0100, Ian Campbell
wrote:
> On Fri, 2012-08-17 at 20:24 +0100, Mukesh Rathor wrote:
> > On Fri, 17 Aug 2012 09:35:54 +0100
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > 
> > > On Thu, 2012-08-16 at 01:57 +0100, Mukesh Rathor wrote:
> > > 
> > > > LDT (linear address, # ents) */
> > > > -    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine
> > > > frames, # ents) */
> > > > +    unsigned long gdt_frames[16], gdt_ents; /* GDT (machine
> > > > frames, # ents).*
> > > > +                                            * PV in HVM:
it''s GDTR
> > > > addr/sz */
> > > 
> > > I''m not sure I understand this comment. What is
"GDTR addr/sz" do you
> > > mean that gdtframes/gdt_ents has a different semantics here?
> > > 
> > > Might be worthy of a union? Or finding some other way to expand
this
> > > struct.
> > 
> > In case of PVH, the field is used to send down GDTR address and size.
> > perhaps better to just leave the comment out.
> 
> I think if the semantics of this field are totally different in the two
> modes then I think a union is warranted.
> 
> > > > -void __init xen_arch_setup(void)
> > > > +/* Normal PV domain not running in HVM container */
> > > 
> > > It''s a bit of a shame to overload the "HVM"
term this way, to mean
> > > both the traditional "providing a full PC like
environment" and "PV
> > > using hardware virtualisation facilities".
> > > 
> > > Perhaps:
> > >         /* Normal PV domain without PVH extensions */
> > 
> > Ok, HVM==Hardware Virtual Machine seems more appropriate here,
> 
> HVM in the context of Xen means more than that though, it implies a Qemu
> and emulation of a complete "PC-like" environment and all of that
stuff,
> which is why I think it is inappropriate/confusing to be overloading it
> to mean "PV with hardware assistance" too.
> 
> Xen''s use of the term HVM is a bit unhelpful, exactly because it
is a
> broad sounding term but with a very specific meaning, but we are kind of
> stuck with it.
> 
> > but I can remove the word HVM and go with ''PVH
extensions''.
> 
> Please ;-)
> 
> > 
> > > >  
> > > >  #ifdef CONFIG_ACPI
> > > >         if (!(xen_start_info->flags &
SIF_INITDOMAIN)) {
> > > > diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
> > > > index f58dca7..cdf269d 100644
> > > > --- a/arch/x86/xen/smp.c
> > > > +++ b/arch/x86/xen/smp.c
> > > > @@ -300,8 +300,6 @@ cpu_initialize_context(unsigned int cpu,
struct
> > > > task_struct *idle) gdt = get_cpu_gdt_table(cpu);
> > > > -       BUG_ON((unsigned long)gdt & ~PAGE_MASK);
> > > > +               ctxt->ldt_ents = 0;
> > > 
> > > Something odd is going on with the indentation here (and below
I''ve
> > > just noticed). I suspect lots of the changes aren''t
really changing
> > > anything other than whitespace?
> > 
> > Konrad wanted just doing the indentation without code change first
> > where it made sense so that further patch makes it easy to see if
> > statements added.
> 
> I disagree that this is a useful way to structure a series unless the
> whitespace change is in a patch of *only* whitespace changes (and even
> then this would be an uncommon way to do things IMO).
That is the intention - as otherwise its darn hard to read what has
changed in the further patches.
> 
> But putting the whitespace changes associated with adding an if
> alongside unrelated actual semantic changes in a totally different patch
> is probably the most confusing and least helpful of all the possible
> options!
<nods> The patch should have been seperate.
> 
> My personal preference would be to do the indent when adding the if and
> let people who want to see the differences without the indentation
> change use "diff -b".
> 
> Konrad is maintainer though, so if he likes this then fine.
> 
> Ian.

On Fri, 17 Aug 2012 15:36:04 -0400
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > > For example in balloon.c we are probably only interested in
memory
> > > related behavior, so checking for XENFEAT_auto_translated_physmap
> > > should be enough.  In other parts of the code we might want to
> > > check for xen_pv_domain(). If xen_pv_domain() and
> > > XENFEAT_auto_translated_physmap are not enough, we could
introduce
> > > another small XENFEAT that specifies that the domain is running
> > > in a HVM container. This way they are all reusable.
> > 
> > yeah, I thought about that, but wasn''t sure what the
implications
> > would be for a guest thats not PVH but has auto xlated physmap, if
> > there''s such a possibility. If you guys think thats not an
issue, I
> > can change it.
> 
> dom0_shadow=on on the hypervisor mode enables that in PV mode.
So, if I just add checks for auto_translated_physmap like suggested,
wouldn''t I be changing and breaking the code paths for dom0_shadow boot
of PV guest? is dom0_shadow depracated?

Following would be true for both, pvh and dom0_shadow:

#define xen_pvh_domain() (xen_pv_domain() && \
                          xen_feature(XENFEAT_auto_translated_physmap)
&& \
                          xen_have_vector_callback)  


Also, the SIF flag allows PVH to be enabled via config file where the
tool pareses and sets it for the guest.

At present:
  dom0: put pvh=true at grub command line
  domU: put pvh=1 in the vm.cfg file.


thanks,
Mukesh

On Fri, 2012-08-17 at 23:26 +0100, Mukesh Rathor wrote:
> On Fri, 17 Aug 2012 15:36:04 -0400
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > > > For example in balloon.c we are probably only interested in
memory
> > > > related behavior, so checking for
XENFEAT_auto_translated_physmap
> > > > should be enough.  In other parts of the code we might want
to
> > > > check for xen_pv_domain(). If xen_pv_domain() and
> > > > XENFEAT_auto_translated_physmap are not enough, we could
introduce
> > > > another small XENFEAT that specifies that the domain is
running
> > > > in a HVM container. This way they are all reusable.
> > > 
> > > yeah, I thought about that, but wasn''t sure what the
implications
> > > would be for a guest thats not PVH but has auto xlated physmap,
if
> > > there''s such a possibility. If you guys think thats not
an issue, I
> > > can change it.
> > 
> > dom0_shadow=on on the hypervisor mode enables that in PV mode.
> 
> So, if I just add checks for auto_translated_physmap like suggested,
> wouldn''t I be changing and breaking the code paths for dom0_shadow
boot
> of PV guest?
Changing, but not breaking, I think. Assuming auto_translated_physmap is
used in the logically correct way.

If anything I think you''d be making dom0_shadow work better, since you
are making stuff actually work.
>  is dom0_shadow depracated?
I hadn''t even heard of it until today.
> 
> Following would be true for both, pvh and dom0_shadow:
> 
> #define xen_pvh_domain() (xen_pv_domain() && \
>                           xen_feature(XENFEAT_auto_translated_physmap)
&& \
>                           xen_have_vector_callback)  
FWIW I don''t think dom0 shadow has vector callback support.

But even if it did we could add a new XENFEAT to allow you to
distinguish if necessary. Lets wait and see what uses of xen_pvh_domain
remain once you converted the easy ones to XENFEAT_writable etc etc. The
remaining uses may show some pattern which we can use to name the new
XENFEAT something more specific than XENFEAT_pvh.

I wonder if PVH deserves a new entry in the XENVER_capabilities string?
> Also, the SIF flag allows PVH to be enabled via config file where the
> tool pareses and sets it for the guest.
> 
> At present:
>   dom0: put pvh=true at grub command line
>   domU: put pvh=1 in the vm.cfg file.
I guess these turn into something like a new XEN_DOMCTL_CDF_* which is
passed to XEN_DOMCTL_createdomain? 

Ian.

On Fri, 17 Aug 2012, Mukesh Rathor wrote:
> On Fri, 17 Aug 2012 15:36:04 -0400
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> 
> > > > For example in balloon.c we are probably only interested in
memory
> > > > related behavior, so checking for
XENFEAT_auto_translated_physmap
> > > > should be enough.  In other parts of the code we might want
to
> > > > check for xen_pv_domain(). If xen_pv_domain() and
> > > > XENFEAT_auto_translated_physmap are not enough, we could
introduce
> > > > another small XENFEAT that specifies that the domain is
running
> > > > in a HVM container. This way they are all reusable.
> > > 
> > > yeah, I thought about that, but wasn''t sure what the
implications
> > > would be for a guest thats not PVH but has auto xlated physmap,
if
> > > there''s such a possibility. If you guys think thats not
an issue, I
> > > can change it.
> > 
> > dom0_shadow=on on the hypervisor mode enables that in PV mode.
> 
> So, if I just add checks for auto_translated_physmap like suggested,
> wouldn''t I be changing and breaking the code paths for dom0_shadow
boot
> of PV guest? is dom0_shadow depracated?
I think that it is just a debugging option. The most recent reference to
dom0_shadow is in 2005, according to Google. Not many people would miss
it.

> Following would be true for both, pvh and dom0_shadow:
> 
> #define xen_pvh_domain() (xen_pv_domain() && \
>                           xen_feature(XENFEAT_auto_translated_physmap)
&& \
>                           xen_have_vector_callback)  
If I understand dom0_shadow correctly, it wouldn''t have
xen_have_vector_callback set, so the above #define would still work as
you expect.
But if all the above characterists are actually true for dom0_shadow
guests too, then it might make sense to call them pvh domains anyway.

> Also, the SIF flag allows PVH to be enabled via config file where the
> tool pareses and sets it for the guest.
> 
> At present:
>   dom0: put pvh=true at grub command line
>   domU: put pvh=1 in the vm.cfg file.
We can still have a pvh option in the VM config file or as a Xen
parameter for dom0: it doesn''t have to be exported as a SIF flag
to the Linux kernel though.
If xen_have_vector_callback is enabled and
XENFEAT_auto_translated_physmap is also set, then we are effectively
running as a PVH domain, otherwise we are not. As a consequence only the
toolstack needs to know about the pvh option in the config file to build
the guest correctly.

On Mon, 20 Aug 2012 12:02:55 +0100
Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 17 Aug 2012, Mukesh Rathor wrote:
> > On Fri, 17 Aug 2012 15:36:04 -0400
> > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > 
> > So, if I just add checks for auto_translated_physmap like suggested,
> > wouldn''t I be changing and breaking the code paths for
dom0_shadow
> > boot of PV guest? is dom0_shadow depracated?
> 
> I think that it is just a debugging option. The most recent reference
> to dom0_shadow is in 2005, according to Google. Not many people would
> miss it.
Agree.
> 
> If I understand dom0_shadow correctly, it wouldn''t have
> xen_have_vector_callback set, so the above #define would still work as
> you expect.
> But if all the above characterists are actually true for dom0_shadow
> guests too, then it might make sense to call them pvh domains anyway.
  
Right.
> 
> We can still have a pvh option in the VM config file or as a Xen
> parameter for dom0: it doesn''t have to be exported as a SIF flag
> to the Linux kernel though.
> If xen_have_vector_callback is enabled and
> XENFEAT_auto_translated_physmap is also set, then we are effectively
> running as a PVH domain, otherwise we are not. As a consequence only
> the toolstack needs to know about the pvh option in the config file
> to build the guest correctly.
Ok, getting rid of SIF flag. The guest will check for above conditions. 

Thanks for the feedback.

On Fri, 17 Aug 2012 09:35:54 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2012-08-16 at 01:57 +0100, Mukesh Rathor wrote:
> 
> > +void __init xen_arch_setup(void)
> > +{
> > +       xen_panic_handler_init();
> > +
> > +       if (!xen_pvh_domain())
> > +               xen_non_pvh_arch_setup();
> 
> The negative in the fn name here strikes me as a bit weird. Can''t
this
> just be xen_pv_arch_setup?
Well, PVH is PV, so xen_pv_arch_setup would be confusing. Thus I can''t
say if (xen_pv_domain()). The negative logic tells the reader right
away that the PV code doesn''t apply to PVH. I earlier had
xen_pure_pv_arch_setup(), but like non_pvh better.
> Or even just have:
> 	/* Everything else is specific to PV without hardware support
> */ if (xen_pvh_domain())
> 		return;
No, the code following if statement is common to both PV and PVH.

On Thu, 2012-08-16 at 01:57 +0100, Mukesh Rathor wrote:
> diff --git a/include/xen/xen.h b/include/xen/xen.h
> index a164024..e823639 100644
> --- a/include/xen/xen.h
> +++ b/include/xen/xen.h
> @@ -18,6 +18,10 @@ extern enum xen_domain_type xen_domain_type;
>                                  xen_domain_type == XEN_PV_DOMAIN)
>  #define xen_hvm_domain()       (xen_domain() &&                   
\
>                                  xen_domain_type == XEN_HVM_DOMAIN)
> +/* xen_pv_domain check is necessary as start_info ptr is null in HVM.
Also,
> + * note, xen PVH domain shares lot of HVM code */
> +#define xen_pvh_domain()       (xen_pv_domain() &&                
\
> +                               (xen_start_info->flags &
SIF_IS_PVINHVM))
Can I suggest that for the time being this be gated on a new
CONFIG_XEN_PVH option (I think it''s new, I can''t find one
right now)
which "depends EXPERIMENTAL".

We don''t want to get into the situation where whatever goes into Linux
now makes it into a distro (and is enabled) and is subsequently broken
on top of whatever the final hypervisor side stuff ends up looking like.
We''ve done the same for the ARM support for example.

Ian.

On Mon, 10 Sep 2012 14:55:52 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2012-08-16 at 01:57 +0100, Mukesh Rathor wrote:
> > diff --git a/include/xen/xen.h b/include/xen/xen.h
> > index a164024..e823639 100644
> > --- a/include/xen/xen.h
> > +++ b/include/xen/xen.h
> > @@ -18,6 +18,10 @@ extern enum xen_domain_type xen_domain_type;
> >                                  xen_domain_type == XEN_PV_DOMAIN)
> >  #define xen_hvm_domain()       (xen_domain()
> > &&                        \ xen_domain_type == XEN_HVM_DOMAIN)
> > +/* xen_pv_domain check is necessary as start_info ptr is null in
> > HVM. Also,
> > + * note, xen PVH domain shares lot of HVM code */
> > +#define xen_pvh_domain()       (xen_pv_domain()
> > &&                     \
> > +                               (xen_start_info->flags &
> > SIF_IS_PVINHVM))
> 
> Can I suggest that for the time being this be gated on a new
> CONFIG_XEN_PVH option (I think it''s new, I can''t find one
right now)
> which "depends EXPERIMENTAL".
> 
> We don''t want to get into the situation where whatever goes into
Linux
> now makes it into a distro (and is enabled) and is subsequently broken
> on top of whatever the final hypervisor side stuff ends up looking
> like. We''ve done the same for the ARM support for example.
> 
> Ian.
Well, I''m pretty much removing all xen_pv_domain() checks and moving to
other checks, like you guys wanted. This code is pretty intermingled and
can''t be ifdef''d easily.

Mukesh

On Tue, 2012-09-11 at 22:57 +0100, Mukesh Rathor wrote:
> On Mon, 10 Sep 2012 14:55:52 +0100
> Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> > On Thu, 2012-08-16 at 01:57 +0100, Mukesh Rathor wrote:
> > > diff --git a/include/xen/xen.h b/include/xen/xen.h
> > > index a164024..e823639 100644
> > > --- a/include/xen/xen.h
> > > +++ b/include/xen/xen.h
> > > @@ -18,6 +18,10 @@ extern enum xen_domain_type xen_domain_type;
> > >                                  xen_domain_type ==
XEN_PV_DOMAIN)
> > >  #define xen_hvm_domain()       (xen_domain()
> > > &&                        \ xen_domain_type ==
XEN_HVM_DOMAIN)
> > > +/* xen_pv_domain check is necessary as start_info ptr is null in
> > > HVM. Also,
> > > + * note, xen PVH domain shares lot of HVM code */
> > > +#define xen_pvh_domain()       (xen_pv_domain()
> > > &&                     \
> > > +                               (xen_start_info->flags &
> > > SIF_IS_PVINHVM))
> > 
> > Can I suggest that for the time being this be gated on a new
> > CONFIG_XEN_PVH option (I think it''s new, I can''t
find one right now)
> > which "depends EXPERIMENTAL".
> > 
> > We don''t want to get into the situation where whatever goes
into Linux
> > now makes it into a distro (and is enabled) and is subsequently broken
> > on top of whatever the final hypervisor side stuff ends up looking
> > like. We''ve done the same for the ARM support for example.
> > 
> > Ian.
> 
> Well, I''m pretty much removing all xen_pv_domain() checks and
moving to
> other checks, like you guys wanted. This code is pretty intermingled and
> can''t be ifdef''d easily.
It would likely be sufficient to just ifdef the bit which tells the
builder that this kernel image supports PVH, which I guess is one or
more XENFEATs in arch/x86/xen/xen-head.S?

Ian.

On Wed, 12 Sep 2012 19:26:20 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2012-09-12 at 19:02 +0100, Mukesh Rathor wrote:
> > On Wed, 12 Sep 2012 09:12:25 +0100
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > 
> > > On Tue, 2012-09-11 at 22:57 +0100, Mukesh Rathor wrote:
> > > > On Mon, 10 Sep 2012 14:55:52 +0100
> > > > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > It would likely be sufficient to just ifdef the bit which tells
> > > the builder that this kernel image supports PVH, which I guess is
> > > one or more XENFEATs in arch/x86/xen/xen-head.S?
> > > 
> > Nop, I''ve not added any new bits. Just using existing bits:
> > 
> > XENFEAT_writable_page_tables
> > XENFEAT_auto_translated_physmap
> > XENFEAT_supervisor_mode_kernel
> 
> Currently we declare:
>         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .asciz
> "!writable_page_tables|pae_pgdir_above_4gb")
> 
> It might be a mistake for writable_page_tables to be there already but
> in any case when you add the other two you should gate them on this
> new option.
> 
> (nb: "!foo" means "foo is required" rather than
"not foo". I chose a
> very confusing character for this!)
> 
> It may also be worth having a XENFEAT_pvh for us in communicating
> support for this feature from the kernel to the tools (like
> XENFEAT_dom0). That''s the opposite of case we wanted to avoid
> (enabling behaviour in the kernel).
Well, the way I have it is, it''s the PV binary. To boot in PVH mode,
you
put pvh=1 in vm.cfg file. The tool stacks read it and tell xen via
new flag XEN_DOMCTL_CDF_hybrid_guest. Err, make that _pvh_guest.
xen sets the new is_pvh bit in struct domain. The PVH guest upon boot
does xen_setup_features() first thing. xen sends it:
> XENFEAT_writable_page_tables
> XENFEAT_auto_translated_physmap
> XENFEAT_supervisor_mode_kernel
 
that with callback vector makes it a PVH guest. 

thanks,
mukesh

On Wed, 2012-09-12 at 20:32 +0100, Mukesh Rathor wrote:
> On Wed, 12 Sep 2012 19:26:20 +0100
> Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> > On Wed, 2012-09-12 at 19:02 +0100, Mukesh Rathor wrote:
> > > On Wed, 12 Sep 2012 09:12:25 +0100
> > > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > 
> > > > On Tue, 2012-09-11 at 22:57 +0100, Mukesh Rathor wrote:
> > > > > On Mon, 10 Sep 2012 14:55:52 +0100
> > > > > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > > It would likely be sufficient to just ifdef the bit which
tells
> > > > the builder that this kernel image supports PVH, which I
guess is
> > > > one or more XENFEATs in arch/x86/xen/xen-head.S?
> > > > 
> > > Nop, I''ve not added any new bits. Just using existing
bits:
> > > 
> > > XENFEAT_writable_page_tables
> > > XENFEAT_auto_translated_physmap
> > > XENFEAT_supervisor_mode_kernel
> > 
> > Currently we declare:
> >         ELFNOTE(Xen, XEN_ELFNOTE_FEATURES,       .asciz
> > "!writable_page_tables|pae_pgdir_above_4gb")
> > 
> > It might be a mistake for writable_page_tables to be there already but
> > in any case when you add the other two you should gate them on this
> > new option.
> > 
> > (nb: "!foo" means "foo is required" rather than
"not foo". I chose a
> > very confusing character for this!)
> > 
> > It may also be worth having a XENFEAT_pvh for us in communicating
> > support for this feature from the kernel to the tools (like
> > XENFEAT_dom0). That''s the opposite of case we wanted to avoid
> > (enabling behaviour in the kernel).
> 
> Well, the way I have it is, it''s the PV binary. To boot in PVH
mode, you
> put pvh=1 in vm.cfg file. The tool stacks read it and tell xen via
> new flag XEN_DOMCTL_CDF_hybrid_guest. Err, make that _pvh_guest.
> xen sets the new is_pvh bit in struct domain. The PVH guest upon boot
> does xen_setup_features() first thing. xen sends it:
> 
> > XENFEAT_writable_page_tables
> > XENFEAT_auto_translated_physmap
> > XENFEAT_supervisor_mode_kernel
>  
> that with callback vector makes it a PVH guest. 
Callback vector is XENFEAT_hvm_callback_vector, I think.

A PV kernel is required to announce the features which it supports via
the elf notes (defined in xen-head.S).

It would be a bug for the toolstack to enable PVH mode for a kernel
which did not claim to support the required featureset.

The aim should be that the user should not normally need to specify pvh
one way or the other, imagine a use case where pygrub is used to boot
both old and new kernels using it.

The toolstack (or the dom0 builder) can make the determination whether
to enable this mode itself itself based on the notes (although it is
always handy to have an override to force things for debug).

Ian.

On Thu, 13 Sep 2012 07:00:07 +0100
Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2012-09-12 at 20:32 +0100, Mukesh Rathor wrote:
> > On Wed, 12 Sep 2012 19:26:20 +0100
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > 
> > > On Wed, 2012-09-12 at 19:02 +0100, Mukesh Rathor wrote:
> Callback vector is XENFEAT_hvm_callback_vector, I think.
> 
> A PV kernel is required to announce the features which it supports via
> the elf notes (defined in xen-head.S).
> 
> It would be a bug for the toolstack to enable PVH mode for a kernel
> which did not claim to support the required featureset.
> 
> The aim should be that the user should not normally need to specify
> pvh one way or the other, imagine a use case where pygrub is used to
> boot both old and new kernels using it.
> 
> The toolstack (or the dom0 builder) can make the determination whether
> to enable this mode itself itself based on the notes (although it is
> always handy to have an override to force things for debug).
Hmmm... So you are suggesting that if hardware and xen supports it, then
PV should just boot in PVH mode and user has no way to override it. I
don''t know if that''s a good idea in the short term, perhaps in
the long
term. Let me discuss with folks here a bit.

thanks,
Mukesh

On Thu, Sep 13, 2012 at 10:14:39AM -0700, Mukesh Rathor
wrote:
> On Thu, 13 Sep 2012 07:00:07 +0100
> Ian Campbell <Ian.Campbell@citrix.com> wrote:
> 
> > On Wed, 2012-09-12 at 20:32 +0100, Mukesh Rathor wrote:
> > > On Wed, 12 Sep 2012 19:26:20 +0100
> > > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > 
> > > > On Wed, 2012-09-12 at 19:02 +0100, Mukesh Rathor wrote:
> > Callback vector is XENFEAT_hvm_callback_vector, I think.
> > 
> > A PV kernel is required to announce the features which it supports via
> > the elf notes (defined in xen-head.S).
> > 
> > It would be a bug for the toolstack to enable PVH mode for a kernel
> > which did not claim to support the required featureset.
> > 
> > The aim should be that the user should not normally need to specify
> > pvh one way or the other, imagine a use case where pygrub is used to
> > boot both old and new kernels using it.
> > 
> > The toolstack (or the dom0 builder) can make the determination whether
> > to enable this mode itself itself based on the notes (although it is
> > always handy to have an override to force things for debug).
> 
> Hmmm... So you are suggesting that if hardware and xen supports it, then
> PV should just boot in PVH mode and user has no way to override it. I
> don''t know if that''s a good idea in the short term,
perhaps in the long
> term. Let me discuss with folks here a bit.
> 
I read that as "if hardware, xen and kernel supports PVH, enable it as a
default,
but still allow the user to force enable/disable it in the domain cfgfile for
easier debugging".

-- Pasi

On Thu, 2012-09-13 at 18:36 +0100, Pasi Kärkkäinen
wrote:
> On Thu, Sep 13, 2012 at 10:14:39AM -0700, Mukesh Rathor wrote:
> > On Thu, 13 Sep 2012 07:00:07 +0100
> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > > The toolstack (or the dom0 builder) can make the determination
whether
> > > to enable this mode itself itself based on the notes (although it
is
> > > always handy to have an override to force things for debug).
> > 
> > Hmmm... So you are suggesting that if hardware and xen supports it,
then
> > PV should just boot in PVH mode and user has no way to override it. I
> > don''t know if that''s a good idea in the short term,
perhaps in the long
> > term. Let me discuss with folks here a bit.
> > 
> 
> I read that as "if hardware, xen and kernel supports PVH, enable it as
a default,
> but still allow the user to force enable/disable it in the domain cfgfile
for easier debugging".
That''s right. I have no objection to an override, in fact I think
it''s a
requirement to be able to force this sort of feature on/off (to aid for
debugging, account for differing workload requirements and allowing the
test system to test both configurations etc).

Long term the aim should be for the default to be on whenever possible.
Maybe the default should remain off for the time being, or maybe we want
to get as much exposure of PVH as possible ASAP, which would argue for
it being on by default in unstable, perhaps with a decision being made
around freeze time as to the default in 4.3.

The feature declarations in the ELF note also allow the toolstack to
sanity check what the user asked for and report an error if it isn''t
possible, rather than booting a kernel in a mode which cannot possibly
work.

Ian.
> 
> -- Pasi
>

On Thu, Sep 13, 2012 at 2:08 PM, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Thu, 2012-09-13 at 18:36 +0100, Pasi Kärkkäinen wrote:
>> On Thu, Sep 13, 2012 at 10:14:39AM -0700, Mukesh Rathor wrote:
>> > On Thu, 13 Sep 2012 07:00:07 +0100
>> > Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > > The toolstack (or the dom0 builder) can make the
determination whether
>> > > to enable this mode itself itself based on the notes
(although it is
>> > > always handy to have an override to force things for debug).
>> >
>> > Hmmm... So you are suggesting that if hardware and xen supports
it, then
>> > PV should just boot in PVH mode and user has no way to override
it. I
>> > don''t know if that''s a good idea in the short
term, perhaps in the long
>> > term. Let me discuss with folks here a bit.
>> >
>>
>> I read that as "if hardware, xen and kernel supports PVH, enable
it as a default,
>> but still allow the user to force enable/disable it in the domain
cfgfile for easier debugging".
>
> That''s right. I have no objection to an override, in fact I think
it''s a
> requirement to be able to force this sort of feature on/off (to aid for
> debugging, account for differing workload requirements and allowing the
> test system to test both configurations etc).
>
> Long term the aim should be for the default to be on whenever possible.
> Maybe the default should remain off for the time being, or maybe we want
> to get as much exposure of PVH as possible ASAP, which would argue for
> it being on by default in unstable, perhaps with a decision being made
> around freeze time as to the default in 4.3.
>
> The feature declarations in the ELF note also allow the toolstack to
> sanity check what the user asked for and report an error if it
isn''t
> possible, rather than booting a kernel in a mode which cannot possibly
> work.
We can also punt the decision to the guest. Meaning it can have one of
those features flags saying: "I_WANT_PVH"

On Thu, 2012-09-13 at 21:45 +0100, Konrad Rzeszutek Wilk
wrote:
> We can also punt the decision to the guest. Meaning it can have one of
> those features flags saying: "I_WANT_PVH"
Right. The ELF notes allow the kernel to specify both optionally
supported and required features.

Ian.