--- arch/x86/xen/enlighten.c | 99 ++++++++++++++++++++++++++++++++++++--------- 1 files changed, 79 insertions(+), 20 deletions(-) diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c index bf4bda6..98f1798 100644 --- a/arch/x86/xen/enlighten.c +++ b/arch/x86/xen/enlighten.c @@ -45,6 +45,7 @@ #include <xen/hvm.h> #include <xen/hvc-console.h> #include <xen/acpi.h> +#include <xen/features.h> #include <asm/paravirt.h> #include <asm/apic.h> @@ -105,6 +106,9 @@ RESERVE_BRK(shared_info_page_brk, PAGE_SIZE); __read_mostly int xen_have_vector_callback; EXPORT_SYMBOL_GPL(xen_have_vector_callback); +#define xen_pvh_domain() (xen_pv_domain() && \ + xen_feature(XENFEAT_auto_translated_physmap) && \ + xen_have_vector_callback) /* * Point at some empty memory to start with. We map the real shared_info * page as soon as fixmap is up and running. @@ -139,6 +143,8 @@ struct tls_descs { */ static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc); +static void __init xen_hvm_init_shared_info(void); + static void clamp_max_cpus(void) { #ifdef CONFIG_SMP @@ -217,8 +223,9 @@ static void __init xen_banner(void) struct xen_extraversion extra; HYPERVISOR_xen_version(XENVER_extraversion, &extra); - printk(KERN_INFO "Booting paravirtualized kernel on %s\n", - pv_info.name); + printk(KERN_INFO "Booting paravirtualized kernel %son %s\n", + xen_feature(XENFEAT_auto_translated_physmap) ? + "with PVH extensions " : "", pv_info.name); printk(KERN_INFO "Xen version: %d.%d%s%s\n", version >> 16, version & 0xffff, extra.extraversion, xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? " (preserve-AD)" : ""); @@ -271,12 +278,15 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx, break; } - asm(XEN_EMULATE_PREFIX "cpuid" - : "=a" (*ax), - "=b" (*bx), - "=c" (*cx), - "=d" (*dx) - : "0" (*ax), "2" (*cx)); + if (xen_pvh_domain()) + native_cpuid(ax, bx, cx, dx); + else + asm(XEN_EMULATE_PREFIX "cpuid" + : "=a" (*ax), + "=b" (*bx), + "=c" (*cx), + "=d" (*dx) + : "0" (*ax), "2" (*cx)); *bx &= maskebx; *cx &= maskecx; @@ -1034,6 +1044,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; + if (!xen_feature(XENFEAT_auto_translated_physmap)) { set_fixmap(FIX_PARAVIRT_BOOTMAP, xen_start_info->shared_info); @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void) HYPERVISOR_shared_info (struct shared_info *)__va(xen_start_info->shared_info); + /* PVH TBD/FIXME: vcpu info placement in phase 2 */ + if (xen_pvh_domain()) + return; + #ifndef CONFIG_SMP /* In UP this is as good a place as any to set up shared info */ xen_setup_vcpu_info_placement(); @@ -1274,6 +1292,11 @@ static const struct machine_ops xen_machine_ops __initconst = { */ static void __init xen_setup_stackprotector(void) { + /* PVH TBD/FIXME: investigate setup_stack_canary_segment */ + if (xen_feature(XENFEAT_auto_translated_physmap)) { + switch_to_new_gdt(0); + return; + } pv_cpu_ops.write_gdt_entry = xen_write_gdt_entry_boot; pv_cpu_ops.load_gdt = xen_load_gdt_boot; @@ -1284,6 +1307,31 @@ static void __init xen_setup_stackprotector(void) pv_cpu_ops.load_gdt = xen_load_gdt; } +static void __init xen_pvh_guest_init(void) +{ + /* PVH TBD/FIXME: for now just disable this. */ + have_vcpu_info_placement = 0; + + /* for domU, the library sets start_info.shared_info to pfn, but for + * dom0, it contains mfn. we need to get the pfn for shared_info. PVH + * uses HVM code in many places */ + if (xen_initial_domain()) + xen_hvm_init_shared_info(); +} + +static void __init xen_pvh_early_guest_init(void) +{ + if (xen_feature(XENFEAT_hvm_callback_vector)) + xen_have_vector_callback = 1; + +#ifdef CONFIG_X86_32 + if (xen_feature(XENFEAT_auto_translated_physmap)) { + xen_raw_printk("ERROR: 32bit PVH guests are not supported\n"); + BUG(); + } +#endif +} + /* First C function to be called on Xen boot */ asmlinkage void __init xen_start_kernel(void) { @@ -1296,13 +1344,18 @@ asmlinkage void __init xen_start_kernel(void) xen_domain_type = XEN_PV_DOMAIN; + xen_setup_features(); + xen_pvh_early_guest_init(); xen_setup_machphys_mapping(); /* Install Xen paravirt ops */ pv_info = xen_info; pv_init_ops = xen_init_ops; - pv_cpu_ops = xen_cpu_ops; pv_apic_ops = xen_apic_ops; + if (xen_pvh_domain()) + pv_cpu_ops.cpuid = xen_cpuid; + else + pv_cpu_ops = xen_cpu_ops; x86_init.resources.memory_setup = xen_memory_setup; x86_init.oem.arch_setup = xen_arch_setup; @@ -1334,8 +1387,6 @@ asmlinkage void __init xen_start_kernel(void) /* Work out if we support NX */ x86_configure_nx(); - xen_setup_features(); - /* Get mfn list */ if (!xen_feature(XENFEAT_auto_translated_physmap)) xen_build_dynamic_phys_to_machine(); @@ -1408,14 +1459,18 @@ asmlinkage void __init xen_start_kernel(void) /* set the limit of our address space */ xen_reserve_top(); - /* We used to do this in xen_arch_setup, but that is too late on AMD - * were early_cpu_init (run before ->arch_setup()) calls early_amd_init - * which pokes 0xcf8 port. - */ - set_iopl.iopl = 1; - rc = HYPERVISOR_physdev_op(PHYSDEVOP_set_iopl, &set_iopl); - if (rc != 0) - xen_raw_printk("physdev_op failed %d\n", rc); + /* PVH: runs at default kernel iopl of 0 */ + if (!xen_pvh_domain()) { + /* + * We used to do this in xen_arch_setup, but that is too late + * on AMD were early_cpu_init (run before ->arch_setup()) calls + * early_amd_init which pokes 0xcf8 port. + */ + set_iopl.iopl = 1; + rc = HYPERVISOR_physdev_op(PHYSDEVOP_set_iopl, &set_iopl); + if (rc != 0) + xen_raw_printk("physdev_op failed %d\n", rc); + } #ifdef CONFIG_X86_32 /* set up basic CPUID stuff */ @@ -1462,6 +1517,9 @@ asmlinkage void __init xen_start_kernel(void) xen_setup_runstate_info(0); + if (xen_pvh_domain()) + xen_pvh_guest_init(); + /* Start the world */ #ifdef CONFIG_X86_32 i386_start_kernel(); @@ -1585,7 +1643,8 @@ static struct syscore_ops xen_hvm_syscore_ops = { }; #endif -/* Use a pfn in RAM, may move to MMIO before kexec. */ +/* Use a pfn in RAM, may move to MMIO before kexec. + * This function also called for PVH dom0 */ static void __init xen_hvm_init_shared_info(void) { /* Remember pointer for resume */ -- 1.7.2.3
Stefano Stabellini
2012-Sep-24 12:07 UTC
Re: [PATCH v1 3/8]: PVH startup changes (enlighten.c)
On Fri, 21 Sep 2012, Mukesh Rathor wrote:> --- > arch/x86/xen/enlighten.c | 99 ++++++++++++++++++++++++++++++++++++--------- > 1 files changed, 79 insertions(+), 20 deletions(-) > > diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c > index bf4bda6..98f1798 100644 > --- a/arch/x86/xen/enlighten.c > +++ b/arch/x86/xen/enlighten.c > @@ -45,6 +45,7 @@ > #include <xen/hvm.h> > #include <xen/hvc-console.h> > #include <xen/acpi.h> > +#include <xen/features.h> > > #include <asm/paravirt.h> > #include <asm/apic.h> > @@ -105,6 +106,9 @@ RESERVE_BRK(shared_info_page_brk, PAGE_SIZE); > __read_mostly int xen_have_vector_callback; > EXPORT_SYMBOL_GPL(xen_have_vector_callback); > > +#define xen_pvh_domain() (xen_pv_domain() && \ > + xen_feature(XENFEAT_auto_translated_physmap) && \ > + xen_have_vector_callback) > /* > * Point at some empty memory to start with. We map the real shared_info > * page as soon as fixmap is up and running. > @@ -139,6 +143,8 @@ struct tls_descs { > */ > static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc); > > +static void __init xen_hvm_init_shared_info(void); > + > static void clamp_max_cpus(void) > { > #ifdef CONFIG_SMP > @@ -217,8 +223,9 @@ static void __init xen_banner(void) > struct xen_extraversion extra; > HYPERVISOR_xen_version(XENVER_extraversion, &extra); > > - printk(KERN_INFO "Booting paravirtualized kernel on %s\n", > - pv_info.name); > + printk(KERN_INFO "Booting paravirtualized kernel %son %s\n", > + xen_feature(XENFEAT_auto_translated_physmap) ? > + "with PVH extensions " : "", pv_info.name); > printk(KERN_INFO "Xen version: %d.%d%s%s\n", > version >> 16, version & 0xffff, extra.extraversion, > xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? " (preserve-AD)" : ""); > @@ -271,12 +278,15 @@ static void xen_cpuid(unsigned int *ax, unsigned int *bx, > break; > } > > - asm(XEN_EMULATE_PREFIX "cpuid" > - : "=a" (*ax), > - "=b" (*bx), > - "=c" (*cx), > - "=d" (*dx) > - : "0" (*ax), "2" (*cx)); > + if (xen_pvh_domain()) > + native_cpuid(ax, bx, cx, dx); > + else > + asm(XEN_EMULATE_PREFIX "cpuid" > + : "=a" (*ax), > + "=b" (*bx), > + "=c" (*cx), > + "=d" (*dx) > + : "0" (*ax), "2" (*cx));can''t we just set the cpuid pvop to native_cpuid?> *bx &= maskebx; > *cx &= maskecx; > @@ -1034,6 +1044,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*/Which one is the function that is going to get called later to do the initialization? It cannot be this one because it would still return immediately.> + if (xen_pvh_domain() && xen_initial_domain()) > + return; > + > if (!xen_feature(XENFEAT_auto_translated_physmap)) { > set_fixmap(FIX_PARAVIRT_BOOTMAP, > xen_start_info->shared_info); > > @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void) > HYPERVISOR_shared_info > (struct shared_info *)__va(xen_start_info->shared_info); > > + /* PVH TBD/FIXME: vcpu info placement in phase 2 */ > + if (xen_pvh_domain()) > + return;It seems that if xen_initial_domain we always skip the initialization while if !xen_initial_domain we only initialize HYPERVISOR_shared_info. I don''t understand why we have this difference. Also it might be worth refactoring xen_setup_shared_info moving out the call to xen_setup_vcpu_info_placement and xen_setup_mfn_list_list in order to avoid all these if ([!]xen_pvh_domain()) return;
On Mon, 24 Sep 2012 13:07:19 +0100 Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:> On Fri, 21 Sep 2012, Mukesh Rathor wrote: > > --- > > arch/x86/xen/enlighten.c | 99 > > ++++++++++++++++++++++++++++++++++++--------- 1 files changed, 79 > > insertions(+), 20 deletions(-) > > > > + "with PVH extensions " : "", pv_info.name); > > printk(KERN_INFO "Xen version: %d.%d%s%s\n", > > version >> 16, version & 0xffff, extra.extraversion, > > xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? > > " (preserve-AD)" : ""); @@ -271,12 +278,15 @@ static void > > xen_cpuid(unsigned int *ax, unsigned int *bx, break; > > } > > > > - asm(XEN_EMULATE_PREFIX "cpuid" > > - : "=a" (*ax), > > - "=b" (*bx), > > - "=c" (*cx), > > - "=d" (*dx) > > - : "0" (*ax), "2" (*cx)); > > + if (xen_pvh_domain()) > > + native_cpuid(ax, bx, cx, dx); > > + else > > + asm(XEN_EMULATE_PREFIX "cpuid" > > + : "=a" (*ax), > > + "=b" (*bx), > > + "=c" (*cx), > > + "=d" (*dx) > > + : "0" (*ax), "2" (*cx)); > > can''t we just set the cpuid pvop to native_cpuid?We had talked about this a bit at code review. There is some massaging before and after that goes on, and so we thought we should just leave it like that.> > > *bx &= maskebx; > > *cx &= maskecx; > > @@ -1034,6 +1044,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*/ > > Which one is the function that is going to get called later to do the > initialization? It cannot be this one because it would still return > immediately.xen_pvh_guest_init() just like the comment says :).> > > + if (xen_pvh_domain() && xen_initial_domain()) > > + return; > > + > > if (!xen_feature(XENFEAT_auto_translated_physmap)) { > > set_fixmap(FIX_PARAVIRT_BOOTMAP, > > xen_start_info->shared_info); > > > > @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void) > > HYPERVISOR_shared_info > > (struct shared_info > > *)__va(xen_start_info->shared_info); > > + /* PVH TBD/FIXME: vcpu info placement in phase 2 */ > > + if (xen_pvh_domain()) > > + return; > > It seems that if xen_initial_domain we always skip the initialization > while if !xen_initial_domain we only initialize > HYPERVISOR_shared_info. I don''t understand why we have this > difference.The comment in xen_pvh_guest_init() explains it. For domU the library maps the pfn at shared_info, ie, shared_info is pfn. For dom0, it''s the mfn. Dom0 then allocates a pfn via extend_brk, and maps the mfn to it. This happens in the commond hvm code, xen_hvm_init_shared_info().
Stefano Stabellini
2012-Sep-25 10:03 UTC
Re: [PATCH v1 3/8]: PVH startup changes (enlighten.c)
On Mon, 24 Sep 2012, Mukesh Rathor wrote:> On Mon, 24 Sep 2012 13:07:19 +0100 > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > > On Fri, 21 Sep 2012, Mukesh Rathor wrote: > > > --- > > > arch/x86/xen/enlighten.c | 99 > > > ++++++++++++++++++++++++++++++++++++--------- 1 files changed, 79 > > > insertions(+), 20 deletions(-) > > > > > > + "with PVH extensions " : "", pv_info.name); > > > printk(KERN_INFO "Xen version: %d.%d%s%s\n", > > > version >> 16, version & 0xffff, extra.extraversion, > > > xen_feature(XENFEAT_mmu_pt_update_preserve_ad) ? > > > " (preserve-AD)" : ""); @@ -271,12 +278,15 @@ static void > > > xen_cpuid(unsigned int *ax, unsigned int *bx, break; > > > } > > > > > > - asm(XEN_EMULATE_PREFIX "cpuid" > > > - : "=a" (*ax), > > > - "=b" (*bx), > > > - "=c" (*cx), > > > - "=d" (*dx) > > > - : "0" (*ax), "2" (*cx)); > > > + if (xen_pvh_domain()) > > > + native_cpuid(ax, bx, cx, dx); > > > + else > > > + asm(XEN_EMULATE_PREFIX "cpuid" > > > + : "=a" (*ax), > > > + "=b" (*bx), > > > + "=c" (*cx), > > > + "=d" (*dx) > > > + : "0" (*ax), "2" (*cx)); > > > > can''t we just set the cpuid pvop to native_cpuid? > > We had talked about this a bit at code review. There is some massaging > before and after that goes on, and so we thought we should just leave > it like that.Ah I see, are you talking about all the masks, correct?> > > + if (xen_pvh_domain() && xen_initial_domain()) > > > + return; > > > + > > > if (!xen_feature(XENFEAT_auto_translated_physmap)) { > > > set_fixmap(FIX_PARAVIRT_BOOTMAP, > > > xen_start_info->shared_info); > > > > > > @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void) > > > HYPERVISOR_shared_info > > > (struct shared_info > > > *)__va(xen_start_info->shared_info); > > > + /* PVH TBD/FIXME: vcpu info placement in phase 2 */ > > > + if (xen_pvh_domain()) > > > + return; > > > > It seems that if xen_initial_domain we always skip the initialization > > while if !xen_initial_domain we only initialize > > HYPERVISOR_shared_info. I don''t understand why we have this > > difference. > > The comment in xen_pvh_guest_init() explains it. For domU the library > maps the pfn at shared_info, ie, shared_info is pfn. For dom0, it''s the > mfn. Dom0 then allocates a pfn via extend_brk, and maps the mfn to it. This > happens in the commond hvm code, xen_hvm_init_shared_info().This difference is really subtle, it would be nice to get rid of it. Could Xen allocate a pfn for dom0? Otherwise could we have the tools allocate an mfn instead of a pfn? In fact looking at xc_dom_x86.c, alloc_magic_pages is explicitly having a different behavior for xc_dom_feature_translated guests and allocates pfn instead of an mfn. Maybe we could get rid of that special case: less code in libxc, a common way of allocating the shared_info page for domU and dom0 => win.
On Tue, 25 Sep 2012 11:03:13 +0100 Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:> On Mon, 24 Sep 2012, Mukesh Rathor wrote: > > On Mon, 24 Sep 2012 13:07:19 +0100 > > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > > > > On Fri, 21 Sep 2012, Mukesh Rathor wrote: > > > > + return; > > > > + > > > > if (!xen_feature(XENFEAT_auto_translated_physmap)) { > > > > set_fixmap(FIX_PARAVIRT_BOOTMAP, > > > > xen_start_info->shared_info); > > > > > > > > @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void) > > > > HYPERVISOR_shared_info > > > > (struct shared_info > > > > *)__va(xen_start_info->shared_info); > > > > + /* PVH TBD/FIXME: vcpu info placement in phase 2 */ > > > > + if (xen_pvh_domain()) > > > > + return; > > > > > > It seems that if xen_initial_domain we always skip the > > > initialization while if !xen_initial_domain we only initialize > > > HYPERVISOR_shared_info. I don''t understand why we have this > > > difference. > > > > The comment in xen_pvh_guest_init() explains it. For domU the > > library maps the pfn at shared_info, ie, shared_info is pfn. For > > dom0, it''s the mfn. Dom0 then allocates a pfn via extend_brk, and > > maps the mfn to it. This happens in the commond hvm code, > > xen_hvm_init_shared_info(). > > This difference is really subtle, it would be nice to get rid of it. > Could Xen allocate a pfn for dom0?Not easily.> Otherwise could we have the tools allocate an mfn instead of a pfn? > In fact looking at xc_dom_x86.c, alloc_magic_pages is explicitly > having a different behavior for xc_dom_feature_translated guests and > allocates pfn instead of an mfn. Maybe we could get rid of that > special case: less code in libxc, a common way of allocating the > shared_info page for domU and dom0 => win.Wish it was simple. But for PV and PVH, domU, it''s already setup the shared page. All we need to do is __va(shared_info). But for HVM domUs and PVH dom0, we need to hcall with pfn to get it remapped. Changing the tool to map pfn, would result in unnecessary hcall for all PV and PVH domUs. It''s only two lines of code, so lets just leave it. I''ll make the comment better. thx, Mukesh
Stefano Stabellini
2012-Sep-26 11:33 UTC
Re: [PATCH v1 3/8]: PVH startup changes (enlighten.c)
On Wed, 26 Sep 2012, Mukesh Rathor wrote:> On Tue, 25 Sep 2012 11:03:13 +0100 > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > > On Mon, 24 Sep 2012, Mukesh Rathor wrote: > > > On Mon, 24 Sep 2012 13:07:19 +0100 > > > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > > > > > > On Fri, 21 Sep 2012, Mukesh Rathor wrote: > > > > > + return; > > > > > + > > > > > if (!xen_feature(XENFEAT_auto_translated_physmap)) { > > > > > set_fixmap(FIX_PARAVIRT_BOOTMAP, > > > > > xen_start_info->shared_info); > > > > > > > > > > @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void) > > > > > HYPERVISOR_shared_info > > > > > (struct shared_info > > > > > *)__va(xen_start_info->shared_info); > > > > > + /* PVH TBD/FIXME: vcpu info placement in phase 2 */ > > > > > + if (xen_pvh_domain()) > > > > > + return; > > > > > > > > It seems that if xen_initial_domain we always skip the > > > > initialization while if !xen_initial_domain we only initialize > > > > HYPERVISOR_shared_info. I don''t understand why we have this > > > > difference. > > > > > > The comment in xen_pvh_guest_init() explains it. For domU the > > > library maps the pfn at shared_info, ie, shared_info is pfn. For > > > dom0, it''s the mfn. Dom0 then allocates a pfn via extend_brk, and > > > maps the mfn to it. This happens in the commond hvm code, > > > xen_hvm_init_shared_info(). > > > > This difference is really subtle, it would be nice to get rid of it. > > Could Xen allocate a pfn for dom0? > > Not easily. > > > Otherwise could we have the tools allocate an mfn instead of a pfn? > > In fact looking at xc_dom_x86.c, alloc_magic_pages is explicitly > > having a different behavior for xc_dom_feature_translated guests and > > allocates pfn instead of an mfn. Maybe we could get rid of that > > special case: less code in libxc, a common way of allocating the > > shared_info page for domU and dom0 => win. > > Wish it was simple. But for PV and PVH, domU, it''s already setup the > shared page. All we need to do is __va(shared_info). But for HVM domUs > and PVH dom0, we need to hcall with pfn to get it remapped.For PVH domU is already setup as a pfn only because in tools/libxc/xc_dom_x86.c:alloc_magic_pages we have this code: if ( xc_dom_feature_translated(dom) ) dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared info"); and in tools/libxc/xc_dom_x86.c:start_info_x86_64 we have: xen_pfn_t shinfo xc_dom_feature_translated(dom) ? dom->shared_info_pfn : dom-> shared_info_mfn; if we simply get rid of the two "if xc_dom_feature_translated(dom)" wouldn''t we get an mfn for PVH domU too? AFAICT all the other cases would remain unmodified, but PVH domU would start getting an mfn for shared_info.> Changing the > tool to map pfn, would result in unnecessary hcall for all PV and PVH > domUs. It''s only two lines of code, so lets just leave it. I''ll make the > comment better.Yes, there would be one more unnecessary hypercall but we would get rid of 4 "if". I think is a good trade off.
On Wed, 2012-09-26 at 12:33 +0100, Stefano Stabellini wrote:> On Wed, 26 Sep 2012, Mukesh Rathor wrote: > > On Tue, 25 Sep 2012 11:03:13 +0100 > > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > > > > On Mon, 24 Sep 2012, Mukesh Rathor wrote: > > > > On Mon, 24 Sep 2012 13:07:19 +0100 > > > > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > > > > > > > > On Fri, 21 Sep 2012, Mukesh Rathor wrote: > > > > > > + return; > > > > > > + > > > > > > if (!xen_feature(XENFEAT_auto_translated_physmap)) { > > > > > > set_fixmap(FIX_PARAVIRT_BOOTMAP, > > > > > > xen_start_info->shared_info); > > > > > > > > > > > > @@ -1044,6 +1058,10 @@ void xen_setup_shared_info(void) > > > > > > HYPERVISOR_shared_info > > > > > > (struct shared_info > > > > > > *)__va(xen_start_info->shared_info); > > > > > > + /* PVH TBD/FIXME: vcpu info placement in phase 2 */ > > > > > > + if (xen_pvh_domain()) > > > > > > + return; > > > > > > > > > > It seems that if xen_initial_domain we always skip the > > > > > initialization while if !xen_initial_domain we only initialize > > > > > HYPERVISOR_shared_info. I don''t understand why we have this > > > > > difference. > > > > > > > > The comment in xen_pvh_guest_init() explains it. For domU the > > > > library maps the pfn at shared_info, ie, shared_info is pfn. For > > > > dom0, it''s the mfn. Dom0 then allocates a pfn via extend_brk, and > > > > maps the mfn to it. This happens in the commond hvm code, > > > > xen_hvm_init_shared_info(). > > > > > > This difference is really subtle, it would be nice to get rid of it. > > > Could Xen allocate a pfn for dom0? > > > > Not easily. > > > > > Otherwise could we have the tools allocate an mfn instead of a pfn? > > > In fact looking at xc_dom_x86.c, alloc_magic_pages is explicitly > > > having a different behavior for xc_dom_feature_translated guests and > > > allocates pfn instead of an mfn. Maybe we could get rid of that > > > special case: less code in libxc, a common way of allocating the > > > shared_info page for domU and dom0 => win. > > > > Wish it was simple. But for PV and PVH, domU, it''s already setup the > > shared page. All we need to do is __va(shared_info). But for HVM domUs > > and PVH dom0, we need to hcall with pfn to get it remapped. > > For PVH domU is already setup as a pfn only because in > tools/libxc/xc_dom_x86.c:alloc_magic_pages we have this code: > > if ( xc_dom_feature_translated(dom) ) > dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared info"); > > and in tools/libxc/xc_dom_x86.c:start_info_x86_64 we have: > > xen_pfn_t shinfo > xc_dom_feature_translated(dom) ? dom->shared_info_pfn : dom-> > shared_info_mfn; > > if we simply get rid of the two "if xc_dom_feature_translated(dom)" > wouldn''t we get an mfn for PVH domU too? AFAICT all the other cases would > remain unmodified, but PVH domU would start getting an mfn for shared_info.The other option would be to have the hypervisor side builder for a PVH dom0 add an entry to the P2M for the shared info as well. Arguably that would be more PV like and therefore the correct thing for a PVH guest since it makes the shared info immediately available to the dom0.> > Changing the > > tool to map pfn, would result in unnecessary hcall for all PV and PVH > > domUs. It''s only two lines of code, so lets just leave it. I''ll make the > > comment better. > > Yes, there would be one more unnecessary hypercall but we would get rid > of 4 "if". I think is a good trade off.Certainly minimising the numbers of hypercalls is not the most important thing in a code path which is run once at start of day.
On Wed, 26 Sep 2012 12:33:39 +0100 Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:> > Wish it was simple. But for PV and PVH, domU, it''s already setup > > the shared page. All we need to do is __va(shared_info). But for > > HVM domUs and PVH dom0, we need to hcall with pfn to get it > > remapped. > > For PVH domU is already setup as a pfn only because in > tools/libxc/xc_dom_x86.c:alloc_magic_pages we have this code: > > if ( xc_dom_feature_translated(dom) ) > dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared info"); > > and in tools/libxc/xc_dom_x86.c:start_info_x86_64 we have: > > xen_pfn_t shinfo > xc_dom_feature_translated(dom) ? dom->shared_info_pfn : dom-> > shared_info_mfn; > > if we simply get rid of the two "if xc_dom_feature_translated(dom)" > wouldn''t we get an mfn for PVH domU too? AFAICT all the other cases > would remain unmodified, but PVH domU would start getting an mfn for > shared_info. > > > Changing the > > tool to map pfn, would result in unnecessary hcall for all PV and > > PVH domUs. It''s only two lines of code, so lets just leave it. I''ll > > make the comment better. > > Yes, there would be one more unnecessary hypercall but we would get > rid of 4 "if". I think is a good trade off.Well, not really unfortunately! There are two fields in the library shared_info_mfn and shared_info_pfn in struct xc_dom_image. For xlated, pfn is set, otherwise, mfn is set. If I set mfn for auto xlated also, I end up changing/adding more code in the library where it tries to map the shared page. Moreover, it''s better to stick with the library assumption that for auto xlated, pfn is set, otherwise, mfn is set. Hope that makes sense. I tried it, btw. thanks, Mukesh
On Tue, 2 Oct 2012 18:36:19 -0700 Mukesh Rathor <mukesh.rathor@oracle.com> wrote:> On Wed, 26 Sep 2012 12:33:39 +0100 > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > > > Wish it was simple. But for PV and PVH, domU, it''s already setup > > > the shared page. All we need to do is __va(shared_info). But for > > > HVM domUs and PVH dom0, we need to hcall with pfn to get it > > > remapped. > > > > For PVH domU is already setup as a pfn only because in > > tools/libxc/xc_dom_x86.c:alloc_magic_pages we have this code: > > > > if ( xc_dom_feature_translated(dom) ) > > dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared > > info"); > > > > and in tools/libxc/xc_dom_x86.c:start_info_x86_64 we have: > > > > xen_pfn_t shinfo > > xc_dom_feature_translated(dom) ? dom->shared_info_pfn : > > dom-> shared_info_mfn; > > > > if we simply get rid of the two "if xc_dom_feature_translated(dom)" > > wouldn''t we get an mfn for PVH domU too? AFAICT all the other cases > > would remain unmodified, but PVH domU would start getting an mfn for > > shared_info. > > > > > Changing the > > > tool to map pfn, would result in unnecessary hcall for all PV and > > > PVH domUs. It''s only two lines of code, so lets just leave it. > > > I''ll make the comment better. > > > > Yes, there would be one more unnecessary hypercall but we would get > > rid of 4 "if". I think is a good trade off. > > Well, not really unfortunately! There are two fields in the library > shared_info_mfn and shared_info_pfn in struct xc_dom_image. For > xlated, pfn is set, otherwise, mfn is set. If I set mfn for auto > xlated also, I end up changing/adding more code in the library where > it tries to map the shared page. Moreover, it''s better to stick with > the library assumption that for auto xlated, pfn is set, otherwise, > mfn is set. > > Hope that makes sense. I tried it, btw. >I''m not able to make a quick change to xen to just put pfn for dom0 also. get_gpfn_from_mfn() is crashing. So for now, how about, lets go with what I''ve got. I''ll make a note, and when I do xen patch, I''ll figure it out, and would be a very small linux patch. I want to get linux patch in soon before the window closes. thanks Mukesh
Stefano Stabellini
2012-Oct-03 11:58 UTC
Re: [PATCH v1 3/8]: PVH startup changes (enlighten.c)
On Wed, 3 Oct 2012, Mukesh Rathor wrote:> On Tue, 2 Oct 2012 18:36:19 -0700 > Mukesh Rathor <mukesh.rathor@oracle.com> wrote: > > > On Wed, 26 Sep 2012 12:33:39 +0100 > > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > > > > > Wish it was simple. But for PV and PVH, domU, it''s already setup > > > > the shared page. All we need to do is __va(shared_info). But for > > > > HVM domUs and PVH dom0, we need to hcall with pfn to get it > > > > remapped. > > > > > > For PVH domU is already setup as a pfn only because in > > > tools/libxc/xc_dom_x86.c:alloc_magic_pages we have this code: > > > > > > if ( xc_dom_feature_translated(dom) ) > > > dom->shared_info_pfn = xc_dom_alloc_page(dom, "shared > > > info"); > > > > > > and in tools/libxc/xc_dom_x86.c:start_info_x86_64 we have: > > > > > > xen_pfn_t shinfo > > > xc_dom_feature_translated(dom) ? dom->shared_info_pfn : > > > dom-> shared_info_mfn; > > > > > > if we simply get rid of the two "if xc_dom_feature_translated(dom)" > > > wouldn''t we get an mfn for PVH domU too? AFAICT all the other cases > > > would remain unmodified, but PVH domU would start getting an mfn for > > > shared_info. > > > > > > > Changing the > > > > tool to map pfn, would result in unnecessary hcall for all PV and > > > > PVH domUs. It''s only two lines of code, so lets just leave it. > > > > I''ll make the comment better. > > > > > > Yes, there would be one more unnecessary hypercall but we would get > > > rid of 4 "if". I think is a good trade off. > > > > Well, not really unfortunately! There are two fields in the library > > shared_info_mfn and shared_info_pfn in struct xc_dom_image. For > > xlated, pfn is set, otherwise, mfn is set. If I set mfn for auto > > xlated also, I end up changing/adding more code in the library where > > it tries to map the shared page. Moreover, it''s better to stick with > > the library assumption that for auto xlated, pfn is set, otherwise, > > mfn is set. > > > > Hope that makes sense. I tried it, btw. > > > > I''m not able to make a quick change to xen to just put pfn for dom0 > also. get_gpfn_from_mfn() is crashing.Did you add the newly allocated shared_info page to dom0''s p2m? If not, the page doesn''t actually belong to the domain (from the p2m POV), so get_gpfn_from_mfn might not behave correctly.> So for now, how about, lets go > with what I''ve got. I''ll make a note, and when I do xen patch, I''ll > figure it out, and would be a very small linux patch. I want to get > linux patch in soon before the window closes.We can leave page special as it is for now with very little consequences, because it is not part of the interface with Xen. However this is part of the interface, so whatever we choose here is going to be hard to change later on. I think it would be good if we could either make dom0 use a pfn or domU use mfn, whatever is easier for you.
On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote:> > So for now, how about, lets go > > with what I''ve got. I''ll make a note, and when I do xen patch, I''ll > > figure it out, and would be a very small linux patch. I want to get > > linux patch in soon before the window closes. > > We can leave page special as it is for now with very little > consequences, because it is not part of the interface with Xen. However > this is part of the interface, so whatever we choose here is going to be > hard to change later on.I suggested that until we have seen and reviewed the hypervisor side patches this guest side functionality all needs to be under an option which has CONFIG_EXPERIMENTAL as a dependency and a comment explaining that the ABI is not fixed yet, similar to what we did for the ARM port. Ian.
Stefano Stabellini
2012-Oct-03 12:36 UTC
Re: [PATCH v1 3/8]: PVH startup changes (enlighten.c)
On Wed, 3 Oct 2012, Ian Campbell wrote:> On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote: > > > So for now, how about, lets go > > > with what I''ve got. I''ll make a note, and when I do xen patch, I''ll > > > figure it out, and would be a very small linux patch. I want to get > > > linux patch in soon before the window closes. > > > > We can leave page special as it is for now with very little > > consequences, because it is not part of the interface with Xen. However > > this is part of the interface, so whatever we choose here is going to be > > hard to change later on. > > I suggested that until we have seen and reviewed the hypervisor side > patches this guest side functionality all needs to be under an option > which has CONFIG_EXPERIMENTAL as a dependency and a comment explaining > that the ABI is not fixed yet, similar to what we did for the ARM port.OK. In that case we can also keep the pfn/mfn issue as it is and change it later.
On Wed, 3 Oct 2012 12:58:22 +0100 Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:> On Wed, 3 Oct 2012, Mukesh Rathor wrote: > > On Tue, 2 Oct 2012 18:36:19 -0700 > > Mukesh Rathor <mukesh.rathor@oracle.com> wrote: > > > > > On Wed, 26 Sep 2012 12:33:39 +0100 > > > > So for now, how about, lets go > > with what I''ve got. I''ll make a note, and when I do xen patch, I''ll > > figure it out, and would be a very small linux patch. I want to get > > linux patch in soon before the window closes. > > We can leave page special as it is for now with very little > consequences, because it is not part of the interface with Xen. > However this is part of the interface, so whatever we choose here is > going to be hard to change later on. > > I think it would be good if we could either make dom0 use a pfn or > domU use mfn, whatever is easier for you.Ok, finally, focussing on this, the issue with pfn in dom0 is that I need pfn allocated in construct_dom0() and be mapped so that the guest can just do : HYPERVISOR_shared_info=(struct shared_info *)__va(xen_start_info->shared_info); How about following I am experimenting with right now: in construct_dom0(): vstartinfo_end = (vstartinfo_start + sizeof(struct start_info) + sizeof(struct dom0_vga_console_info)); if ( is_hybrid_domain(d) ) { start_info_pfn_addr = round_pgup(vstartinfo_end) - v_start; vstartinfo_end += PAGE_SIZE; } I can then put (PFN: start_info_pfn_addr)->(MFN: virt_to_maddr(d->shared_info)) in the p2m, and dom0 just has to do __va(), like domU does now. I wont'' need to special case dom0 then. Do you foresee any problems with this approach? thanks Mukesh
On Wed, 2012-10-03 at 23:37 +0100, Mukesh Rathor wrote:> On Wed, 3 Oct 2012 12:58:22 +0100 > Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote: > > > On Wed, 3 Oct 2012, Mukesh Rathor wrote: > > > On Tue, 2 Oct 2012 18:36:19 -0700 > > > Mukesh Rathor <mukesh.rathor@oracle.com> wrote: > > > > > > > On Wed, 26 Sep 2012 12:33:39 +0100 > > > > > > > So for now, how about, lets go > > > with what I''ve got. I''ll make a note, and when I do xen patch, I''ll > > > figure it out, and would be a very small linux patch. I want to get > > > linux patch in soon before the window closes. > > > > We can leave page special as it is for now with very little > > consequences, because it is not part of the interface with Xen. > > However this is part of the interface, so whatever we choose here is > > going to be hard to change later on. > > > > I think it would be good if we could either make dom0 use a pfn or > > domU use mfn, whatever is easier for you. > > Ok, finally, focussing on this, the issue with pfn in dom0 is that > I need pfn allocated in construct_dom0() and be mapped so that the > guest can just do : > > HYPERVISOR_shared_info=(struct shared_info *)__va(xen_start_info->shared_info); > > How about following I am experimenting with right now: > > in construct_dom0(): > > vstartinfo_end = (vstartinfo_start + > sizeof(struct start_info) + > sizeof(struct dom0_vga_console_info)); > > if ( is_hybrid_domain(d) ) { > start_info_pfn_addr = round_pgup(vstartinfo_end) - v_start; > vstartinfo_end += PAGE_SIZE; > } > > I can then put (PFN: start_info_pfn_addr)->(MFN: virt_to_maddr(d->shared_info)) > in the p2m, and dom0 just has to do __va(), like domU does now. I wont'' need > to special case dom0 then. > > Do you foresee any problems with this approach?Hard to say without all the surrounding context but it seems plausible to me. Ian.
On Thu, 4 Oct 2012 09:38:40 +0100 Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Wed, 2012-10-03 at 23:37 +0100, Mukesh Rathor wrote: > > On Wed, 3 Oct 2012 12:58:22 +0100 > > Ok, finally, focussing on this, the issue with pfn in dom0 is that > > I need pfn allocated in construct_dom0() and be mapped so that the > > guest can just do : > > > > HYPERVISOR_shared_info=(struct shared_info > > *)__va(xen_start_info->shared_info); > > > > How about following I am experimenting with right now: > > > > in construct_dom0(): > > > > vstartinfo_end = (vstartinfo_start + > > sizeof(struct start_info) + > > sizeof(struct dom0_vga_console_info)); > > > > if ( is_hybrid_domain(d) ) { > > start_info_pfn_addr = round_pgup(vstartinfo_end) - v_start; > > vstartinfo_end += PAGE_SIZE; > > } > > > > I can then put (PFN: start_info_pfn_addr)->(MFN: > > virt_to_maddr(d->shared_info)) in the p2m, and dom0 just has to do > > __va(), like domU does now. I wont'' need to special case dom0 then. > > > > Do you foresee any problems with this approach? > > Hard to say without all the surrounding context but it seems plausible > to me.Ok, above works. So no dom0 special case in linux now to map shared_page. thanks Mukesh
On Wed, 3 Oct 2012 13:05:14 +0100 Ian Campbell <Ian.Campbell@citrix.com> wrote:> On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote: > > > So for now, how about, lets go > > > with what I''ve got. I''ll make a note, and when I do xen patch, > > > I''ll figure it out, and would be a very small linux patch. I want > > > to get linux patch in soon before the window closes. > > > > We can leave page special as it is for now with very little > > consequences, because it is not part of the interface with Xen. > > However this is part of the interface, so whatever we choose here > > is going to be hard to change later on. > > I suggested that until we have seen and reviewed the hypervisor side > patches this guest side functionality all needs to be under an option > which has CONFIG_EXPERIMENTAL as a dependency and a comment explaining > that the ABI is not fixed yet, similar to what we did for the ARM > port.Konrad is going to keep the changes in his tree and not upstream for couple months while we go thru the xen patches. This would be a big help for me as I won''t have to constantly refresh linux tree. I hope we can get some baseline in linux and xen soon. thanks Mukesh
On Fri, 2012-10-05 at 02:35 +0100, Mukesh Rathor wrote:> On Wed, 3 Oct 2012 13:05:14 +0100 > Ian Campbell <Ian.Campbell@citrix.com> wrote: > > > On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote: > > > > So for now, how about, lets go > > > > with what I''ve got. I''ll make a note, and when I do xen patch, > > > > I''ll figure it out, and would be a very small linux patch. I want > > > > to get linux patch in soon before the window closes. > > > > > > We can leave page special as it is for now with very little > > > consequences, because it is not part of the interface with Xen. > > > However this is part of the interface, so whatever we choose here > > > is going to be hard to change later on. > > > > I suggested that until we have seen and reviewed the hypervisor side > > patches this guest side functionality all needs to be under an option > > which has CONFIG_EXPERIMENTAL as a dependency and a comment explaining > > that the ABI is not fixed yet, similar to what we did for the ARM > > port. > > Konrad is going to keep the changes in his tree and not upstream for > couple months while we go thru the xen patches. This would be a big > help for me as I won''t have to constantly refresh linux tree.Ah, ok, that sounds like a good plan. In the case where the ARM stuff I have built on this becomes ripe first would it be OK with you guys if I were to cherry pick the relevant bits of the PVH stuff into a series to go in sooner?> I hope we can get some baseline in linux and xen soon.Likewise! Ian.
Konrad Rzeszutek Wilk
2012-Oct-08 12:41 UTC
Re: [PATCH v1 3/8]: PVH startup changes (enlighten.c)
On Fri, Oct 05, 2012 at 10:23:25AM +0100, Ian Campbell wrote:> On Fri, 2012-10-05 at 02:35 +0100, Mukesh Rathor wrote: > > On Wed, 3 Oct 2012 13:05:14 +0100 > > Ian Campbell <Ian.Campbell@citrix.com> wrote: > > > > > On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote: > > > > > So for now, how about, lets go > > > > > with what I''ve got. I''ll make a note, and when I do xen patch, > > > > > I''ll figure it out, and would be a very small linux patch. I want > > > > > to get linux patch in soon before the window closes. > > > > > > > > We can leave page special as it is for now with very little > > > > consequences, because it is not part of the interface with Xen. > > > > However this is part of the interface, so whatever we choose here > > > > is going to be hard to change later on. > > > > > > I suggested that until we have seen and reviewed the hypervisor side > > > patches this guest side functionality all needs to be under an option > > > which has CONFIG_EXPERIMENTAL as a dependency and a comment explaining > > > that the ABI is not fixed yet, similar to what we did for the ARM > > > port. > > > > Konrad is going to keep the changes in his tree and not upstream for > > couple months while we go thru the xen patches. This would be a big > > help for me as I won''t have to constantly refresh linux tree. > > Ah, ok, that sounds like a good plan. > > In the case where the ARM stuff I have built on this becomes ripe first > would it be OK with you guys if I were to cherry pick the relevant bits > of the PVH stuff into a series to go in sooner?My plan right now was to wait for Mukesh to repost his series (which I hope will address all our review comments) and then I can shovel any other patches on top of that. But if it makes sense to intermix Mukeshes''s patchs, and yours. (or squash some together) I can do that too.
On Mon, 8 Oct 2012 08:41:02 -0400 Konrad Rzeszutek Wilk <konrad@kernel.org> wrote:> On Fri, Oct 05, 2012 at 10:23:25AM +0100, Ian Campbell wrote: > > On Fri, 2012-10-05 at 02:35 +0100, Mukesh Rathor wrote: > > > On Wed, 3 Oct 2012 13:05:14 +0100 > > > Ian Campbell <Ian.Campbell@citrix.com> wrote: > > > > > > > On Wed, 2012-10-03 at 12:58 +0100, Stefano Stabellini wrote: > > In the case where the ARM stuff I have built on this becomes ripe > > first would it be OK with you guys if I were to cherry pick the > > relevant bits of the PVH stuff into a series to go in sooner? > > My plan right now was to wait for Mukesh to repost his series (which > I hope will address all our review comments) and then I can shovel > any other patches on top of that. > > But if it makes sense to intermix Mukeshes''s patchs, and yours. > (or squash some together) I can do that too.Ok, I''m all done with changes. Doing final testing now, and will have patches sent by Tues afternoon. thanks, Mukesh