Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1323252 > unrolled thread

[PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-02-01 16:50 +0100
    Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest David Vrabel <david.vrabel@citrix.com> - 2016-02-02 17:40 +0100
      Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-02-02 18:20 +0100
    Re: [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-02-03 20:00 +0100
      Re: [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-02-03 21:20 +0100
        Re: [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest "Luis R. Rodriguez" <mcgrof@suse.com> - 2016-02-04 00:50 +0100
          Re: [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-02-04 21:00 +0100
            Re: [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest "Luis R. Rodriguez" <mcgrof@suse.com> - 2016-02-04 22:00 +0100
              Re: [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2016-02-04 23:30 +0100
      Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest Andrew Cooper <andrew.cooper3@citrix.com> - 2016-02-03 22:00 +0100
        Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest "Luis R. Rodriguez" <mcgrof@suse.com> - 2016-02-04 01:00 +0100
          Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest "Luis R. Rodriguez" <mcgrof@kernel.org> - 2016-02-04 01:10 +0100
          Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest Andrew Cooper <andrew.cooper3@citrix.com> - 2016-02-04 02:00 +0100
            Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest "Luis R. Rodriguez" <mcgrof@suse.com> - 2016-02-05 00:20 +0100

#1323252 — [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest

Start HVMlite guest at XEN_ELFNOTE_PHYS32_ENTRY address. Setup hypercall
page, initialize boot_params, enable early page tables.

Since this stub is executed before kernel entry point we cannot use
variables in .bss which is cleared by kernel. We explicitly place
variables that are initialized here into .data.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 arch/x86/xen/Makefile      |    1 +
 arch/x86/xen/enlighten.c   |   86 +++++++++++++++++++++-
 arch/x86/xen/xen-hvmlite.S |  175 ++++++++++++++++++++++++++++++++++++++++++++
 include/xen/xen.h          |    6 ++
 4 files changed, 267 insertions(+), 1 deletions(-)
 create mode 100644 arch/x86/xen/xen-hvmlite.S

diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
index e47e527..1d913d7 100644
--- a/arch/x86/xen/Makefile
+++ b/arch/x86/xen/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_XEN_DEBUG_FS)	+= debugfs.o
 obj-$(CONFIG_XEN_DOM0)		+= vga.o
 obj-$(CONFIG_SWIOTLB_XEN)	+= pci-swiotlb-xen.o
 obj-$(CONFIG_XEN_EFI)		+= efi.o
+obj-$(CONFIG_XEN_PVHVM) 	+= xen-hvmlite.o
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 5774800..5f05fa2 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -118,7 +118,8 @@ DEFINE_PER_CPU(struct vcpu_info *, xen_vcpu);
  */
 DEFINE_PER_CPU(struct vcpu_info, xen_vcpu_info);
 
-enum xen_domain_type xen_domain_type = XEN_NATIVE;
+enum xen_domain_type xen_domain_type
+	__attribute__((section(".data"))) = XEN_NATIVE;
 EXPORT_SYMBOL_GPL(xen_domain_type);
 
 unsigned long *machine_to_phys_mapping = (void *)MACH2PHYS_VIRT_START;
@@ -171,6 +172,17 @@ struct tls_descs {
  */
 static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc);
 
+#ifdef CONFIG_XEN_PVHVM
+/*
+ * HVMlite variables. These need to live in data segment since they are
+ * initialized before startup_{32|64}, which clear .bss, are invoked.
+ */
+int xen_hvmlite __attribute__((section(".data"))) = 0;
+struct hvm_start_info hvmlite_start_info __attribute__((section(".data")));
+uint hvmlite_start_info_sz = sizeof(hvmlite_start_info);
+struct boot_params xen_hvmlite_boot_params __attribute__((section(".data")));
+#endif
+
 static void clamp_max_cpus(void)
 {
 #ifdef CONFIG_SMP
@@ -1731,6 +1743,78 @@ asmlinkage __visible void __init xen_start_kernel(void)
 #endif
 }
 
+#ifdef CONFIG_XEN_PVHVM
+static void __init hvmlite_bootparams(void)
+{
+	struct xen_memory_map memmap;
+	int i;
+
+	memset(&xen_hvmlite_boot_params, 0, sizeof(xen_hvmlite_boot_params));
+
+	memmap.nr_entries = ARRAY_SIZE(xen_hvmlite_boot_params.e820_map);
+	set_xen_guest_handle(memmap.buffer, xen_hvmlite_boot_params.e820_map);
+	if (HYPERVISOR_memory_op(XENMEM_memory_map, &memmap)) {
+		xen_raw_console_write("XENMEM_memory_map failed\n");
+		BUG();
+	}
+
+	xen_hvmlite_boot_params.e820_map[memmap.nr_entries].addr =
+		ISA_START_ADDRESS;
+	xen_hvmlite_boot_params.e820_map[memmap.nr_entries].size =
+		ISA_END_ADDRESS - ISA_START_ADDRESS;
+	xen_hvmlite_boot_params.e820_map[memmap.nr_entries++].type =
+		E820_RESERVED;
+
+	sanitize_e820_map(xen_hvmlite_boot_params.e820_map,
+			  ARRAY_SIZE(xen_hvmlite_boot_params.e820_map),
+			  &memmap.nr_entries);
+
+	xen_hvmlite_boot_params.e820_entries = memmap.nr_entries;
+	for (i = 0; i < xen_hvmlite_boot_params.e820_entries; i++)
+		e820_add_region(xen_hvmlite_boot_params.e820_map[i].addr,
+				xen_hvmlite_boot_params.e820_map[i].size,
+				xen_hvmlite_boot_params.e820_map[i].type);
+
+	xen_hvmlite_boot_params.hdr.cmd_line_ptr =
+		hvmlite_start_info.cmdline_paddr;
+
+	/* The first module is always ramdisk */
+	if (hvmlite_start_info.nr_modules) {
+		struct hvm_modlist_entry *modaddr =
+			__va(hvmlite_start_info.modlist_paddr);
+		xen_hvmlite_boot_params.hdr.ramdisk_image = modaddr->paddr;
+		xen_hvmlite_boot_params.hdr.ramdisk_size = modaddr->size;
+	}
+
+	/*
+	 * See Documentation/x86/boot.txt.
+	 *
+	 * Version 2.12 supports Xen entry point but we will use default x86/PC
+	 * environment (i.e. hardware_subarch 0).
+	 */
+	xen_hvmlite_boot_params.hdr.version = 0x212;
+	xen_hvmlite_boot_params.hdr.type_of_loader = 9; /* Xen loader */
+}
+
+/*
+ * This routine (and those that it might call) should not use
+ * anything that lives in .bss since that segment will be cleared later
+ */
+void __init xen_prepare_hvmlite(void)
+{
+	u32 eax, ecx, edx, msr;
+	u64 pfn;
+
+	xen_hvmlite = 1;
+
+	cpuid(xen_cpuid_base() + 2, &eax, &msr, &ecx, &edx);
+	pfn = __pa(hypercall_page);
+	wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
+
+	hvmlite_bootparams();
+}
+#endif
+
 void __ref xen_hvm_init_shared_info(void)
 {
 	int cpu;
diff --git a/arch/x86/xen/xen-hvmlite.S b/arch/x86/xen/xen-hvmlite.S
new file mode 100644
index 0000000..fc7c08c
--- /dev/null
+++ b/arch/x86/xen/xen-hvmlite.S
@@ -0,0 +1,175 @@
+/*
+ * Copyright C 2016, Oracle and/or its affiliates. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+	.code32
+	.text
+#define _pa(x)          ((x) - __START_KERNEL_map)
+
+#include <linux/elfnote.h>
+#include <linux/init.h>
+#include <linux/linkage.h>
+#include <asm/segment.h>
+#include <asm/asm.h>
+#include <asm/boot.h>
+#include <asm/processor-flags.h>
+#include <asm/msr.h>
+#include <xen/interface/elfnote.h>
+
+	__HEAD
+	.code32
+
+/* Entry point for HVMlite guests */
+ENTRY(hvmlite_start_xen)
+	cli
+	cld
+
+	mov $_pa(gdt), %eax
+	lgdt (%eax)
+
+	movl $(__BOOT_DS),%eax
+	movl %eax,%ds
+	movl %eax,%es
+	movl %eax,%ss
+
+	/* Stash hvm_start_info */
+	mov $_pa(hvmlite_start_info), %edi
+	mov %ebx, %esi
+	mov $_pa(hvmlite_start_info_sz), %ecx
+	mov (%ecx), %ecx
+	rep
+	movsb
+
+	movl $_pa(early_stack_end), %eax
+	movl %eax, %esp
+
+	/* Enable PAE mode */
+	movl %cr4, %eax
+	orl $X86_CR4_PAE, %eax
+	movl %eax, %cr4
+
+#ifdef CONFIG_X86_64
+	/* Enable Long mode */
+	movl $MSR_EFER, %ecx
+	rdmsr
+	btsl $_EFER_LME, %eax
+	wrmsr
+
+	/* Enable pre-constructed page tables */
+	mov $_pa(init_level4_pgt), %eax
+	movl %eax, %cr3
+	movl $(X86_CR0_PG | X86_CR0_PE), %eax
+	movl %eax, %cr0
+
+	/* Jump to 64-bit mode. */
+	pushl $__KERNEL_CS
+	leal _pa(1f), %eax
+	pushl %eax
+	lret
+
+	/* 64-bit entry point */
+	.code64
+1:
+	call xen_prepare_hvmlite
+
+	/* startup_64 expects boot_params in %rsi */
+	mov $_pa(xen_hvmlite_boot_params), %rsi
+	movq $_pa(startup_64), %rax
+	jmp *%rax
+
+#else /* CONFIG_X86_64 */
+
+	/* Clear boot page tables */
+	movl $_pa(early_pgtable), %edi
+	xorl %eax, %eax
+	movl $((PAGE_SIZE*5)/4), %ecx
+	rep stosl
+
+	/* Level 3 */
+	movl $_pa(early_pgtable), %edi
+	leal (PAGE_SIZE +_PAGE_PRESENT)(%edi), %eax
+	movl $4, %ecx
+1:
+	movl %eax, 0x00(%edi)
+	addl $8, %edi
+	decl %ecx
+	jnz 1b
+
+	/* Level 2 (2M entries) */
+	movl $(_pa(early_pgtable) + PAGE_SIZE), %edi
+	movl $(_PAGE_PSE | _PAGE_RW | _PAGE_PRESENT), %eax
+	movl $2048, %ecx
+2:
+	movl %eax, 0(%edi)
+	addl $0x00200000, %eax
+	addl $8, %edi
+	decl %ecx
+	jnz 2b
+
+	/* Enable the boot paging */
+	movl $_pa(early_pgtable), %eax
+	movl %eax, %cr3
+	movl %cr0, %eax
+	orl $(X86_CR0_PG | X86_CR0_PE), %eax
+	movl %eax, %cr0
+
+	ljmp $__BOOT_CS,$3f
+3:
+	call xen_prepare_hvmlite
+	mov $_pa(xen_hvmlite_boot_params), %esi
+
+	/* startup_32 doesn't expect paging and PAE to be on */
+	ljmp $__BOOT_CS,$_pa(4f)
+4:
+	movl %cr0, %eax
+	andl $~X86_CR0_PG, %eax
+	movl %eax, %cr0
+	movl %cr4, %eax
+	andl $~X86_CR4_PAE, %eax
+	movl %eax, %cr4
+
+	ljmp    $0x10, $_pa(startup_32)
+#endif
+
+	.data
+gdt:
+	.word	gdt_end - gdt
+	.long	_pa(gdt)
+	.word	0
+	.quad	0x0000000000000000 /* NULL descriptor */
+#ifdef CONFIG_X86_64
+	.quad	0x00af9a000000ffff /* __KERNEL_CS */
+#else
+	.quad	0x00cf9a000000ffff /* __KERNEL_CS */
+#endif
+	.quad	0x00cf92000000ffff /* __KERNEL_DS */
+gdt_end:
+
+	.bss
+	.balign 4
+early_stack:
+	.fill 16, 1, 0
+early_stack_end:
+
+#ifdef CONFIG_X86_32
+	.section ".pgtable","a",@nobits
+	.balign 4096
+early_pgtable:
+	.fill 5*4096, 1, 0
+#endif
+
+	ELFNOTE(Xen, XEN_ELFNOTE_PHYS32_ENTRY,
+	             _ASM_PTR (hvmlite_start_xen - __START_KERNEL_map))
diff --git a/include/xen/xen.h b/include/xen/xen.h
index 0c0e3ef..6a0d3f3 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -29,6 +29,12 @@ extern enum xen_domain_type xen_domain_type;
 #define xen_initial_domain()	(0)
 #endif	/* CONFIG_XEN_DOM0 */
 
+#ifdef CONFIG_XEN_PVHVM
+extern int xen_hvmlite;
+#else
+#define xen_hvmlite		(0)
+#endif
+
 #ifdef CONFIG_XEN_PVH
 /* This functionality exists only for x86. The XEN_PVHVM support exists
  * only in x86 world - hence on ARM it will be always disabled.
-- 
1.7.1

[toc] | [next] | [standalone]

#1324257 — Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest

On 01/02/16 15:38, Boris Ostrovsky wrote:
> --- a/include/xen/xen.h
> +++ b/include/xen/xen.h
> @@ -29,6 +29,12 @@ extern enum xen_domain_type xen_domain_type;
>  #define xen_initial_domain()	(0)
>  #endif	/* CONFIG_XEN_DOM0 */
>  
> +#ifdef CONFIG_XEN_PVHVM
> +extern int xen_hvmlite;
> +#else
> +#define xen_hvmlite		(0)
> +#endif

I think we want a feature set and not a single boolean here.  There's
going to HVMlite variants (e.g., those with an emulated APIC to support
PCI passthrough and those without).

So we test e.g., xen_hvm_feature(XEN_HVM_FEATURE_APIC), which would be
clear for a HVMlite guest without an (emulated) APIC.

David

[toc] | [prev] | [next] | [standalone]

#1324300 — Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest

On 02/02/2016 11:39 AM, David Vrabel wrote:
> On 01/02/16 15:38, Boris Ostrovsky wrote:
>> --- a/include/xen/xen.h
>> +++ b/include/xen/xen.h
>> @@ -29,6 +29,12 @@ extern enum xen_domain_type xen_domain_type;
>>   #define xen_initial_domain()	(0)
>>   #endif	/* CONFIG_XEN_DOM0 */
>>   
>> +#ifdef CONFIG_XEN_PVHVM
>> +extern int xen_hvmlite;
>> +#else
>> +#define xen_hvmlite		(0)
>> +#endif
> I think we want a feature set and not a single boolean here.  There's
> going to HVMlite variants (e.g., those with an emulated APIC to support
> PCI passthrough and those without).
>
> So we test e.g., xen_hvm_feature(XEN_HVM_FEATURE_APIC), which would be
> clear for a HVMlite guest without an (emulated) APIC.

I was thinking of eventually 's/xen_hvmlite/xen_pvh_domain/g' but this 
is better.

Then we need to export features via XENVER_get_features. Alternatively, 
we can provide them in the hypervisor CPUID leaf.

-boris

[toc] | [prev] | [next] | [standalone]

#1325863

I saw no considerations for the recommendations I had made last on your v1:

https://lkml.kernel.org/r/CAB=NE6XPA0YzbnM8=rspkKai6d3GkXXO00Gr0VZUYoyzNy6thw@mail.gmail.com

Of importance:

1) Using pv_info.paravirt_enabled = 1 is wrong unless you mean to say this
   is for legacy x86:

Your patch #3 keeps on setting pv_info.paravirt_enabled = 1 and as discussed
this is wrong. It will be renamed to x86_legacy_free() to align with what folks
are pushing for a BIOS flag to annotate if a system requires legacy x86 stuff.
This also means re-thinking all use cases and ensuring subarch is used then
instead when the goal was to avoid Xen from entering that code. Today Xen does
not use this but with my work it does and it helps clean and brush up a lot of
these checks with future prospects to even help unify entry points.

2) We should avoid more hypervisor type hacks, and just consider a new 
   hypervisor type to close the gap:

Using x86_legacy_free() and friends in a unified way for all systems means it
should only be used after init_hypervisor_platform() which is called during
setup_arch().  This means we have a semantic gap for checks on "are we on
hypervisor type and which one?". There are drivers now using these sorts of
checks as well, for instance snd_intel8x0_inside_vm(). We should avoid having
these hacks but that also means cleaning up a well define grammar here for what
we want.  I'm doing work to help with this by streamlining use of the subarch
type, that should help with PV code, but your use case seems different but yet
related, what I had suggested last was to consider we add a new hypervisor type
to the x86 boot protocol which would be available early on. This would have a
few purposes, one of which deserves its own section below on dead code:

    a) clean up hacks as with snd_intel8x0_inside_vm()
    b) enable a generic way and clean way to distinguish what hypervisor
       type you're on
    c) since it would be set early and if we can ensure its accessible
       early on boot it would mean avoiding having to add yet-another
       asm entry point for Linux, you could just use startup_32() and
       the hypervisor type could easily just have an early branch call
       and post branch call very similar to how we deal with the subarch
       currently on 32-bit. Your calls then just become early stubs and
       we'd have a solution for other PV types that want a similar solution
       later

3) Dead code concerns and unifying entry points:

Addressing the semantics for the gray areas I am highlighting are critical for
ensuring one does not run code or even exposes code as a available for the type
of run time system booted, some folks call this "dead code". This is critical
for Linux distributions which need to rely on the flexibility of having one
kernel work for different use cases. The resolution to this problem was pvops
but pvops has shortcoming for dead code, it didn't address the problem likely as
it was not considered serious. It also didn't address the issue of different
hypervisors wanting different entry points and that this fact alone also contributes
to more dead code concerns, case in point the regressions introduced by cr4 shadow
and the latest one is Kasan which to this day breaks Xen! Dead code topics are
not easy to grasp, its why I've started on my own crusade to talk to people and
write about it [0], and as of late propose some changes to avoid these in a
clean way without extending pvops. Adding yet another entry point will not help
here *specially* if we do not take semantics seriously over the different hypervisors
and hypervisor types.

[0] http://www.do-not-panic.com/2015/12/avoiding-dead-code-pvops-not-silver-bullet.html
[1] http://www.do-not-panic.com/2015/12/xen-and-x86-linux-zero-page.html

My recommendation then:

We add new hypervisor type to close the semantic gap for hypervisor types, and
much like subarch enable also a subarch_data to let you pass and use your
hvmlite_start_info. This would not only help with the semantics but also help
avoid yet-another-entry point and force us to provide a well define structure
for considering code that should not run by pegging it as required or supported
for different early x86 code stubs.

I had hinted perhaps we might be able to piggy back on top of the ELF loader
protocol as well, and since that's standard do wonder if that could instead
be extended to help unify a mechanism for different OSes instead of making
this just a solution for Linux.

Code review below.

On Mon, Feb 01, 2016 at 10:38:48AM -0500, Boris Ostrovsky wrote:
> Start HVMlite guest at XEN_ELFNOTE_PHYS32_ENTRY address. Setup hypercall
> page, initialize boot_params, enable early page tables.
> 
> Since this stub is executed before kernel entry point we cannot use
> variables in .bss which is cleared by kernel. We explicitly place
> variables that are initialized here into .data.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 5774800..5f05fa2 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -171,6 +172,17 @@ struct tls_descs {
>   */
>  static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc);
>  
> +#ifdef CONFIG_XEN_PVHVM
> +/*
> + * HVMlite variables. These need to live in data segment since they are
> + * initialized before startup_{32|64}, which clear .bss, are invoked.
> + */
> +int xen_hvmlite __attribute__((section(".data"))) = 0;
> +struct hvm_start_info hvmlite_start_info __attribute__((section(".data")));
> +uint hvmlite_start_info_sz = sizeof(hvmlite_start_info);
> +struct boot_params xen_hvmlite_boot_params __attribute__((section(".data")));
> +#endif
> +

The section annotations seems very special use case but likely worth documenting
and defining a new macro for in include/linux/compiler.h. This would make it
easier to change should we want to change the section used here later and
enable others to easily look for the reason for these annotations in a
single place.

  Luis

[toc] | [prev] | [next] | [standalone]

#1325907

On 02/03/2016 01:55 PM, Luis R. Rodriguez wrote:
> I saw no considerations for the recommendations I had made last on your v1:
>
> https://lkml.kernel.org/r/CAB=NE6XPA0YzbnM8=rspkKai6d3GkXXO00Gr0VZUYoyzNy6thw@mail.gmail.com
>
> Of importance:
>
> 1) Using pv_info.paravirt_enabled = 1 is wrong unless you mean to say this
>     is for legacy x86:
>
> Your patch #3 keeps on setting pv_info.paravirt_enabled = 1 and as discussed
> this is wrong. It will be renamed to x86_legacy_free() to align with what folks
> are pushing for a BIOS flag to annotate if a system requires legacy x86 stuff.
> This also means re-thinking all use cases and ensuring subarch is used then
> instead when the goal was to avoid Xen from entering that code. Today Xen does
> not use this but with my work it does and it helps clean and brush up a lot of
> these checks with future prospects to even help unify entry points.

As I said earlier, I am not sure I understand what subarch buys us for 
HVMlite guests.

As for using paravirt_enabled -- this is really only used to 
differentiate HVM from HVMlite and I think (although I'd need to check) 
is only needed by Xen-specific code in a couple of places. So if/when it 
is removed we will switch to something else. Since your work is WIP I 
decided to keep using it until it's clear what other options may be 
available.

>
> 2) We should avoid more hypervisor type hacks, and just consider a new
>     hypervisor type to close the gap:
>
> Using x86_legacy_free() and friends in a unified way for all systems means it
> should only be used after init_hypervisor_platform() which is called during
> setup_arch().  This means we have a semantic gap for checks on "are we on
> hypervisor type and which one?".

In this particular case we don't need any information about hypervisor 
until init_hypervisor_platform().

> There are drivers now using these sorts of
> checks as well, for instance snd_intel8x0_inside_vm(). We should avoid having
> these hacks but that also means cleaning up a well define grammar here for what
> we want.  I'm doing work to help with this by streamlining use of the subarch
> type, that should help with PV code, but your use case seems different but yet
> related, what I had suggested last was to consider we add a new hypervisor type
> to the x86 boot protocol which would be available early on. This would have a
> few purposes, one of which deserves its own section below on dead code:
>
>      a) clean up hacks as with snd_intel8x0_inside_vm()
>      b) enable a generic way and clean way to distinguish what hypervisor
>         type you're on
>      c) since it would be set early and if we can ensure its accessible
>         early on boot it would mean avoiding having to add yet-another
>         asm entry point for Linux, you could just use startup_32() and
>         the hypervisor type could easily just have an early branch call
>         and post branch call very similar to how we deal with the subarch
>         currently on 32-bit. Your calls then just become early stubs and
>         we'd have a solution for other PV types that want a similar solution
>         later

Which calls?

If you are referring to xen_prepare_hvmlite/hvmlite_bootparams then 
these are needed to prepare boot_params. And we should not enter 
startup_32() without them ready.


>
> 3) Dead code concerns and unifying entry points:
>
> Addressing the semantics for the gray areas I am highlighting are critical for
> ensuring one does not run code or even exposes code as a available for the type
> of run time system booted, some folks call this "dead code". This is critical
> for Linux distributions which need to rely on the flexibility of having one
> kernel work for different use cases. The resolution to this problem was pvops
> but pvops has shortcoming for dead code, it didn't address the problem likely as
> it was not considered serious. It also didn't address the issue of different
> hypervisors wanting different entry points and that this fact alone also contributes
> to more dead code concerns, case in point the regressions introduced by cr4 shadow
> and the latest one is Kasan which to this day breaks Xen! Dead code topics are
> not easy to grasp, its why I've started on my own crusade to talk to people and
> write about it [0], and as of late propose some changes to avoid these in a
> clean way without extending pvops. Adding yet another entry point will not help
> here *specially* if we do not take semantics seriously over the different hypervisors
> and hypervisor types.
>
> [0] http://www.do-not-panic.com/2015/12/avoiding-dead-code-pvops-not-silver-bullet.html
> [1] http://www.do-not-panic.com/2015/12/xen-and-x86-linux-zero-page.html

I don't understand what this has to do with HVMlite guests.

>
> My recommendation then:
>
> We add new hypervisor type to close the semantic gap for hypervisor types, and
> much like subarch enable also a subarch_data to let you pass and use your
> hvmlite_start_info. This would not only help with the semantics but also help
> avoid yet-another-entry point and force us to provide a well define structure
> for considering code that should not run by pegging it as required or supported
> for different early x86 code stubs.

As I said before, I don't see how we can avoid having another entry 
point without making Xen change its load procedure. Which is highly 
unlikely to happen.

>
> I had hinted perhaps we might be able to piggy back on top of the ELF loader
> protocol as well, and since that's standard do wonder if that could instead
> be extended to help unify a mechanism for different OSes instead of making
> this just a solution for Linux.
>
> Code review below.
>
> On Mon, Feb 01, 2016 at 10:38:48AM -0500, Boris Ostrovsky wrote:
>> Start HVMlite guest at XEN_ELFNOTE_PHYS32_ENTRY address. Setup hypercall
>> page, initialize boot_params, enable early page tables.
>>
>> Since this stub is executed before kernel entry point we cannot use
>> variables in .bss which is cleared by kernel. We explicitly place
>> variables that are initialized here into .data.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
>> index 5774800..5f05fa2 100644
>> --- a/arch/x86/xen/enlighten.c
>> +++ b/arch/x86/xen/enlighten.c
>> @@ -171,6 +172,17 @@ struct tls_descs {
>>    */
>>   static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc);
>>   
>> +#ifdef CONFIG_XEN_PVHVM
>> +/*
>> + * HVMlite variables. These need to live in data segment since they are
>> + * initialized before startup_{32|64}, which clear .bss, are invoked.
>> + */
>> +int xen_hvmlite __attribute__((section(".data"))) = 0;
>> +struct hvm_start_info hvmlite_start_info __attribute__((section(".data")));
>> +uint hvmlite_start_info_sz = sizeof(hvmlite_start_info);
>> +struct boot_params xen_hvmlite_boot_params __attribute__((section(".data")));
>> +#endif
>> +
> The section annotations seems very special use case but likely worth documenting
> and defining a new macro for in include/linux/compiler.h. This would make it
> easier to change should we want to change the section used here later and
> enable others to easily look for the reason for these annotations in a
> single place.

I wonder whether __initdata would be a good attribute. We only need this 
early in the boot.

And xen_hvmlite is gone now so we don't need to worry about it.

-boris

[toc] | [prev] | [next] | [standalone]

#1326267

On Wed, Feb 03, 2016 at 03:11:56PM -0500, Boris Ostrovsky wrote:
> On 02/03/2016 01:55 PM, Luis R. Rodriguez wrote:
> >I saw no considerations for the recommendations I had made last on your v1:
> >
> >https://lkml.kernel.org/r/CAB=NE6XPA0YzbnM8=rspkKai6d3GkXXO00Gr0VZUYoyzNy6thw@mail.gmail.com
> >
> >Of importance:
> >
> >1) Using pv_info.paravirt_enabled = 1 is wrong unless you mean to say this
> >    is for legacy x86:
> >
> >Your patch #3 keeps on setting pv_info.paravirt_enabled = 1 and as discussed
> >this is wrong. It will be renamed to x86_legacy_free() to align with what folks
> >are pushing for a BIOS flag to annotate if a system requires legacy x86 stuff.
> >This also means re-thinking all use cases and ensuring subarch is used then
> >instead when the goal was to avoid Xen from entering that code. Today Xen does
> >not use this but with my work it does and it helps clean and brush up a lot of
> >these checks with future prospects to even help unify entry points.
> 
> As I said earlier, I am not sure I understand what subarch buys us
> for HVMlite guests.

I accepted subarch may not be the right thing, so proposed a hypervisor type.
What it buys you is a strong semantics association between code designed
for a purpose.

> As for using paravirt_enabled -- this is really only used to
> differentiate HVM from HVMlite and I think (although I'd need to
> check) is only needed by Xen-specific code in a couple of places.

That sounds like a Xen specific use case as such an interface that is
pointed out as going to renamed to reflect its actual use case should not
be abused for that purpose.

> So if/when it is removed we will switch to something else. Since your work is
> WIP I decided to keep using it until it's clear what other options may be
> available.

And your work is not WIP? I'll be splitting my patches up and the rename
will be atomic, it likely can go in first than yours, so not sure why you
are simply brushing this off.

> >2) We should avoid more hypervisor type hacks, and just consider a new
> >    hypervisor type to close the gap:
> >
> >Using x86_legacy_free() and friends in a unified way for all systems means it
> >should only be used after init_hypervisor_platform() which is called during
> >setup_arch().  This means we have a semantic gap for checks on "are we on
> >hypervisor type and which one?".
> 
> In this particular case we don't need any information about
> hypervisor until init_hypervisor_platform().

I pointed out in your v1 patchset how microcode loading was not blocked, you
then asked how KVM does it, and that was explained as well, and that they
don't enable it as well. You need a solution for this.

> >There are drivers now using these sorts of
> >checks as well, for instance snd_intel8x0_inside_vm(). We should avoid having
> >these hacks but that also means cleaning up a well define grammar here for what
> >we want.  I'm doing work to help with this by streamlining use of the subarch
> >type, that should help with PV code, but your use case seems different but yet
> >related, what I had suggested last was to consider we add a new hypervisor type
> >to the x86 boot protocol which would be available early on. This would have a
> >few purposes, one of which deserves its own section below on dead code:
> >
> >     a) clean up hacks as with snd_intel8x0_inside_vm()
> >     b) enable a generic way and clean way to distinguish what hypervisor
> >        type you're on
> >     c) since it would be set early and if we can ensure its accessible
> >        early on boot it would mean avoiding having to add yet-another
> >        asm entry point for Linux, you could just use startup_32() and
> >        the hypervisor type could easily just have an early branch call
> >        and post branch call very similar to how we deal with the subarch
> >        currently on 32-bit. Your calls then just become early stubs and
> >        we'd have a solution for other PV types that want a similar solution
> >        later
> 
> Which calls?
> 
> If you are referring to xen_prepare_hvmlite/hvmlite_bootparams

Even before, hvmlite_start_xen(). 

> then these are needed to prepare boot_params. And we should not enter
> startup_32() without them ready.

As-is the x86 boot protocol would not allow an easy way for this, I'm
suggesting we consider extending the boot protocol to add a hypervisor
type and data pointer much as with subarch and subarch_data for the
particular purpose of both enabling entry into the same startup_32()
but also a clean way for modifications of stubs both at the beginning
and at the end of startup_32().

Pseudo code:

startup_32()                         startup_64()
       |                                  |
       |                                  |
       V                                  V
pre_hypervisor_stub_32()	pre_hypervisor_stub_64()
       |                                  |
       |                                  |
       V                                  V
 [existing startup_32()]       [existing startup_64()]
       |                                  |
       |                                  |
       V                                  V
post_hypervisor_stub_32()	post_hypervisor_stub_64()

The pre_hypervisor_stub_32() would have much of the code in
hvmlite_start_xen() but for 32-bit, pre_hypervisor_stub_64()
would have the 64-bits.

> >3) Dead code concerns and unifying entry points:
> >
> >Addressing the semantics for the gray areas I am highlighting are critical for
> >ensuring one does not run code or even exposes code as a available for the type
> >of run time system booted, some folks call this "dead code". This is critical
> >for Linux distributions which need to rely on the flexibility of having one
> >kernel work for different use cases. The resolution to this problem was pvops
> >but pvops has shortcoming for dead code, it didn't address the problem likely as
> >it was not considered serious. It also didn't address the issue of different
> >hypervisors wanting different entry points and that this fact alone also contributes
> >to more dead code concerns, case in point the regressions introduced by cr4 shadow
> >and the latest one is Kasan which to this day breaks Xen! Dead code topics are
> >not easy to grasp, its why I've started on my own crusade to talk to people and
> >write about it [0], and as of late propose some changes to avoid these in a
> >clean way without extending pvops. Adding yet another entry point will not help
> >here *specially* if we do not take semantics seriously over the different hypervisors
> >and hypervisor types.
> >
> >[0] http://www.do-not-panic.com/2015/12/avoiding-dead-code-pvops-not-silver-bullet.html
> >[1] http://www.do-not-panic.com/2015/12/xen-and-x86-linux-zero-page.html
> 
> I don't understand what this has to do with HVMlite guests.

We have no formal semantics to ensure correctness and avoid dead code for HVMLite,
as with PV. The microcode loader is one example of code that should not run. Sure,
an adhoc solution might be possible, but I'm advocating for a generic solution.

> >My recommendation then:
> >
> >We add new hypervisor type to close the semantic gap for hypervisor types, and
> >much like subarch enable also a subarch_data to let you pass and use your
> >hvmlite_start_info. This would not only help with the semantics but also help
> >avoid yet-another-entry point and force us to provide a well define structure
> >for considering code that should not run by pegging it as required or supported
> >for different early x86 code stubs.
> 
> As I said before, I don't see how we can avoid having another entry
> point without making Xen change its load procedure. Which is highly
> unlikely to happen.

Why would it not be possible if this is about a new guest type? Surely there
are good reasons to consider extending the load procedure protocol?

> >I had hinted perhaps we might be able to piggy back on top of the ELF loader
> >protocol as well, and since that's standard do wonder if that could instead
> >be extended to help unify a mechanism for different OSes instead of making
> >this just a solution for Linux.

If it was possible to extend it, would this be a reasonable venue to consider?

> >Code review below.
> >
> >On Mon, Feb 01, 2016 at 10:38:48AM -0500, Boris Ostrovsky wrote:
> >>Start HVMlite guest at XEN_ELFNOTE_PHYS32_ENTRY address. Setup hypercall
> >>page, initialize boot_params, enable early page tables.
> >>
> >>Since this stub is executed before kernel entry point we cannot use
> >>variables in .bss which is cleared by kernel. We explicitly place
> >>variables that are initialized here into .data.
> >>
> >>Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >>---
> >>diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> >>index 5774800..5f05fa2 100644
> >>--- a/arch/x86/xen/enlighten.c
> >>+++ b/arch/x86/xen/enlighten.c
> >>@@ -171,6 +172,17 @@ struct tls_descs {
> >>   */
> >>  static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc);
> >>+#ifdef CONFIG_XEN_PVHVM
> >>+/*
> >>+ * HVMlite variables. These need to live in data segment since they are
> >>+ * initialized before startup_{32|64}, which clear .bss, are invoked.
> >>+ */
> >>+int xen_hvmlite __attribute__((section(".data"))) = 0;
> >>+struct hvm_start_info hvmlite_start_info __attribute__((section(".data")));
> >>+uint hvmlite_start_info_sz = sizeof(hvmlite_start_info);
> >>+struct boot_params xen_hvmlite_boot_params __attribute__((section(".data")));
> >>+#endif
> >>+
> >The section annotations seems very special use case but likely worth documenting
> >and defining a new macro for in include/linux/compiler.h. This would make it
> >easier to change should we want to change the section used here later and
> >enable others to easily look for the reason for these annotations in a
> >single place.
> 
> I wonder whether __initdata would be a good attribute. We only need
> this early in the boot.

I could not find other users of .data other than some specific driver.
Using anything with *init* alludes you can free the data later but if we
want to keep it I suggest a different prefix, up to you.

  Luis

[toc] | [prev] | [next] | [standalone]

#1327145

On 02/03/2016 06:40 PM, Luis R. Rodriguez wrote:
> On Wed, Feb 03, 2016 at 03:11:56PM -0500, Boris Ostrovsky wrote:
>> On 02/03/2016 01:55 PM, Luis R. Rodriguez wrote:
>>> I saw no considerations for the recommendations I had made last on your v1:
>>>
>>> https://lkml.kernel.org/r/CAB=NE6XPA0YzbnM8=rspkKai6d3GkXXO00Gr0VZUYoyzNy6thw@mail.gmail.com
>>>
>>> Of importance:
>>>
>>> 1) Using pv_info.paravirt_enabled = 1 is wrong unless you mean to say this
>>>     is for legacy x86:
>>>
>>> Your patch #3 keeps on setting pv_info.paravirt_enabled = 1 and as discussed
>>> this is wrong. It will be renamed to x86_legacy_free() to align with what folks
>>> are pushing for a BIOS flag to annotate if a system requires legacy x86 stuff.
>>> This also means re-thinking all use cases and ensuring subarch is used then
>>> instead when the goal was to avoid Xen from entering that code. Today Xen does
>>> not use this but with my work it does and it helps clean and brush up a lot of
>>> these checks with future prospects to even help unify entry points.
>> As I said earlier, I am not sure I understand what subarch buys us
>> for HVMlite guests.
> I accepted subarch may not be the right thing, so proposed a hypervisor type.

I don't see much difference between having an HV-specific subarch and a 
hypervisor type.

> What it buys you is a strong semantics association between code designed
> for a purpose.
>
>> As for using paravirt_enabled -- this is really only used to
>> differentiate HVM from HVMlite and I think (although I'd need to
>> check) is only needed by Xen-specific code in a couple of places.
> That sounds like a Xen specific use case as such an interface that is
> pointed out as going to renamed to reflect its actual use case should not
> be abused for that purpose.
>
>> So if/when it is removed we will switch to something else. Since your work is
>> WIP I decided to keep using it until it's clear what other options may be
>> available.
> And your work is not WIP? I'll be splitting my patches up and the rename
> will be atomic, it likely can go in first than yours, so not sure why you
> are simply brushing this off.

I didn't mean to imply anything by saying that your patches are a WIP. 
It's just that I can only write and test my patches against existing 
code, not the future one.

I am sorry if you felt I was trying to say something else, it certainly 
was not my intent.

>
>>> 2) We should avoid more hypervisor type hacks, and just consider a new
>>>     hypervisor type to close the gap:
>>>
>>> Using x86_legacy_free() and friends in a unified way for all systems means it
>>> should only be used after init_hypervisor_platform() which is called during
>>> setup_arch().  This means we have a semantic gap for checks on "are we on
>>> hypervisor type and which one?".
>> In this particular case we don't need any information about
>> hypervisor until init_hypervisor_platform().
> I pointed out in your v1 patchset how microcode loading was not blocked, you
> then asked how KVM does it, and that was explained as well, and that they
> don't enable it as well. You need a solution for this.

Not really. Xen will ignore writes to microcode-specific MSRs, just like 
KVM.

This is exact same behavior we have now with regular HVM guests.


> As-is the x86 boot protocol would not allow an easy way for this, I'm 
> suggesting we consider extending the boot protocol to add a hypervisor 
> type and data pointer much as with subarch and subarch_data for the

Who will set hypervisor type and where? It won't be Xen as Andrew 
mentioned in another email.

> particular purpose of both enabling entry into the same startup_32()
> but also a clean way for modifications of stubs both at the beginning
> and at the end of startup_32().
>
> Pseudo code:
>
> startup_32()                         startup_64()
>         |                                  |
>         |                                  |
>         V                                  V
> pre_hypervisor_stub_32()	pre_hypervisor_stub_64()
>         |                                  |
>         |                                  |
>         V                                  V
>   [existing startup_32()]       [existing startup_64()]
>         |                                  |
>         |                                  |
>         V                                  V
> post_hypervisor_stub_32()	post_hypervisor_stub_64()
>
> The pre_hypervisor_stub_32() would have much of the code in
> hvmlite_start_xen() but for 32-bit, pre_hypervisor_stub_64()
> would have the 64-bits.


Sure. When the protocol is agreed upon and this code is written we will 
just move hvmlite_start_xen() to pre_hypervisor_stub_32().



> +int xen_hvmlite __attribute__((section(".data"))) = 0;
> +struct hvm_start_info hvmlite_start_info __attribute__((section(".data")));
> +uint hvmlite_start_info_sz = sizeof(hvmlite_start_info);
> +struct boot_params xen_hvmlite_boot_params __attribute__((section(".data")));
> +#endif
> +
>>> The section annotations seems very special use case but likely worth documenting
>>> and defining a new macro for in include/linux/compiler.h. This would make it
>>> easier to change should we want to change the section used here later and
>>> enable others to easily look for the reason for these annotations in a
>>> single place.
>> I wonder whether __initdata would be a good attribute. We only need
>> this early in the boot.
> I could not find other users of .data other than some specific driver.
> Using anything with *init* alludes you can free the data later but if we
> want to keep it I suggest a different prefix, up to you.

That's why I said that we only need this info early in the boot.

-boris

[toc] | [prev] | [next] | [standalone]

#1327181

On Thu, Feb 04, 2016 at 02:54:15PM -0500, Boris Ostrovsky wrote:
> On 02/03/2016 06:40 PM, Luis R. Rodriguez wrote:
> >On Wed, Feb 03, 2016 at 03:11:56PM -0500, Boris Ostrovsky wrote:
> >>On 02/03/2016 01:55 PM, Luis R. Rodriguez wrote:
> >>>I saw no considerations for the recommendations I had made last on your v1:
> >>>
> >>>https://lkml.kernel.org/r/CAB=NE6XPA0YzbnM8=rspkKai6d3GkXXO00Gr0VZUYoyzNy6thw@mail.gmail.com
> >>>
> >>>Of importance:
> >>>
> >>>1) Using pv_info.paravirt_enabled = 1 is wrong unless you mean to say this
> >>>    is for legacy x86:
> >>>
> >>>Your patch #3 keeps on setting pv_info.paravirt_enabled = 1 and as discussed
> >>>this is wrong. It will be renamed to x86_legacy_free() to align with what folks
> >>>are pushing for a BIOS flag to annotate if a system requires legacy x86 stuff.
> >>>This also means re-thinking all use cases and ensuring subarch is used then
> >>>instead when the goal was to avoid Xen from entering that code. Today Xen does
> >>>not use this but with my work it does and it helps clean and brush up a lot of
> >>>these checks with future prospects to even help unify entry points.
> >>As I said earlier, I am not sure I understand what subarch buys us
> >>for HVMlite guests.
> >I accepted subarch may not be the right thing, so proposed a hypervisor type.
> 
> I don't see much difference between having an HV-specific subarch
> and a hypervisor type.

Ah, well here lies the issue. As per hpa subarch was not designed for defining
a hypervisor, but rather at least subarch PC (0) [should be used if the
hardware is] "enumerable using standard PC mechanisms (PCI, ACPI) and doesn't
need a special boot flow". Does that follow the definition of HVMlite?

I was pointing out to hpa how paravirt_enabled() has limitations in that it is
set late and as such only logically be available for all users after
setup_arch(), so I figured we could repurpose subarch for a hypervisor type.
He noted:

  "If you have a genuine need for a "hypervisor type" then that is a
   separate thing and should be treated separately from subarch.  However,
   you need to consider that some hypervisors can emulate other hypervisors
   and you may have more than one hypervisor API available."

> >What it buys you is a strong semantics association between code designed
> >for a purpose.
> >
> >>As for using paravirt_enabled -- this is really only used to
> >>differentiate HVM from HVMlite and I think (although I'd need to
> >>check) is only needed by Xen-specific code in a couple of places.
> >That sounds like a Xen specific use case as such an interface that is
> >pointed out as going to renamed to reflect its actual use case should not
> >be abused for that purpose.
> >
> >>So if/when it is removed we will switch to something else. Since your work is
> >>WIP I decided to keep using it until it's clear what other options may be
> >>available.
> >And your work is not WIP? I'll be splitting my patches up and the rename
> >will be atomic, it likely can go in first than yours, so not sure why you
> >are simply brushing this off.
> 
> I didn't mean to imply anything by saying that your patches are a
> WIP. It's just that I can only write and test my patches against
> existing code, not the future one.
> 
> I am sorry if you felt I was trying to say something else, it
> certainly was not my intent.

I don't really care about that, my point was that we both are working on
similar areas right now and both efforts are helping us clean up the init
path and give us better semantics, we should take both patch series into
consideration as they *both* are being reviewed now. The definition and use
of subarch at least of importance here for HVMLite in consideration for
future cleanup.

> >>>2) We should avoid more hypervisor type hacks, and just consider a new
> >>>    hypervisor type to close the gap:
> >>>
> >>>Using x86_legacy_free() and friends in a unified way for all systems means it
> >>>should only be used after init_hypervisor_platform() which is called during
> >>>setup_arch().  This means we have a semantic gap for checks on "are we on
> >>>hypervisor type and which one?".
> >>In this particular case we don't need any information about
> >>hypervisor until init_hypervisor_platform().
> >I pointed out in your v1 patchset how microcode loading was not blocked, you
> >then asked how KVM does it, and that was explained as well, and that they
> >don't enable it as well. You need a solution for this.
> 
> Not really. Xen will ignore writes to microcode-specific MSRs, just
> like KVM.
>
> This is exact same behavior we have now with regular HVM guests.

OK great. That still means the code will run, and if we can avoid that
why not. I am fine with annotating this as future work to help. Let me
then ask as well, how about the rest of the code during and after
startup_32() and startup_64() -- are we sure that's all safe ?

> >As-is the x86 boot protocol would not allow an easy way for this,
> >I'm suggesting we consider extending the boot protocol to add a
> >hypervisor type and data pointer much as with subarch and
> >subarch_data for the
> 
> Who will set hypervisor type and where? It won't be Xen as Andrew
> mentioned in another email.

Andrew seems to think I'm after some senseless prodding, there are
good reasons to consider setting at least a type and custom data
pointer, and in fact I think there are gains for this not only for
Linux but other OSes; so I'll keep working on my arguments there.

> >particular purpose of both enabling entry into the same startup_32()
> >but also a clean way for modifications of stubs both at the beginning
> >and at the end of startup_32().
> >
> >Pseudo code:
> >
> >startup_32()                         startup_64()
> >        |                                  |
> >        |                                  |
> >        V                                  V
> >pre_hypervisor_stub_32()	pre_hypervisor_stub_64()
> >        |                                  |
> >        |                                  |
> >        V                                  V
> >  [existing startup_32()]       [existing startup_64()]
> >        |                                  |
> >        |                                  |
> >        V                                  V
> >post_hypervisor_stub_32()	post_hypervisor_stub_64()
> >
> >The pre_hypervisor_stub_32() would have much of the code in
> >hvmlite_start_xen() but for 32-bit, pre_hypervisor_stub_64()
> >would have the 64-bits.
> 
> 
> Sure. When the protocol is agreed upon and this code is written we
> will just move hvmlite_start_xen() to pre_hypervisor_stub_32().

OK fair enough.

> >+int xen_hvmlite __attribute__((section(".data"))) = 0;
> >+struct hvm_start_info hvmlite_start_info __attribute__((section(".data")));
> >+uint hvmlite_start_info_sz = sizeof(hvmlite_start_info);
> >+struct boot_params xen_hvmlite_boot_params __attribute__((section(".data")));
> >+#endif
> >+
> >>>The section annotations seems very special use case but likely worth documenting
> >>>and defining a new macro for in include/linux/compiler.h. This would make it
> >>>easier to change should we want to change the section used here later and
> >>>enable others to easily look for the reason for these annotations in a
> >>>single place.
> >>I wonder whether __initdata would be a good attribute. We only need
> >>this early in the boot.
> >I could not find other users of .data other than some specific driver.
> >Using anything with *init* alludes you can free the data later but if we
> >want to keep it I suggest a different prefix, up to you.
> 
> That's why I said that we only need this info early in the boot.

Still -- better just document and add a shared macro for it.

  Luis

[toc] | [prev] | [next] | [standalone]

#1327235

On 02/04/2016 03:57 PM, Luis R. Rodriguez wrote:
>
> Ah, well here lies the issue. As per hpa subarch was not designed for defining
> a hypervisor, but rather at least subarch PC (0) [should be used if the
> hardware is] "enumerable using standard PC mechanisms (PCI, ACPI) and doesn't
> need a special boot flow". Does that follow the definition of HVMlite?

Yes. HVMlite is going to use baremetal boot flow.

> OK great. That still means the code will run, and if we can avoid that
> why not. I am fine with annotating this as future work to help. Let me
> then ask as well, how about the rest of the code during and after
> startup_32() and startup_64() -- are we sure that's all safe ?

I can't be sure, all I can say is that so far I haven't seen any problems.

-boris

[toc] | [prev] | [next] | [standalone]

#1325942 — Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest

On 03/02/16 18:55, Luis R. Rodriguez wrote:
> We add new hypervisor type to close the semantic gap for hypervisor types, and
> much like subarch enable also a subarch_data to let you pass and use your
> hvmlite_start_info. This would not only help with the semantics but also help
> avoid yet-another-entry point and force us to provide a well define structure
> for considering code that should not run by pegging it as required or supported
> for different early x86 code stubs.

Was I unclear last time?  Xen *will not* be introducing Linux-specifics
into the HVMLite starting ABI.

Your perceived problem with multiple entry points is not a problem with
multiple entry points; It is a problem with multiple different paths
performing the same initialisation.

The Linux entry for PV guests is indeed completely horrible.  I am not
trying to defend it in the slightest.

However, the HVMLite entry which is a very short stub that sets up a
zeropage and hands off to the native start routine is fine.  There is
still just routine performing native x86 startup.

If you still desperately want to avoid multiple entry points, then just
insist on using grub for the VM.  I expect that that is how most people
will end up using HVMLite VMs anyway.

~Andrew

[toc] | [prev] | [next] | [standalone]

#1326268 — Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest

On Wed, Feb 03, 2016 at 08:52:50PM +0000, Andrew Cooper wrote:
> On 03/02/16 18:55, Luis R. Rodriguez wrote:
> > We add new hypervisor type to close the semantic gap for hypervisor types, and
> > much like subarch enable also a subarch_data to let you pass and use your
> > hvmlite_start_info. This would not only help with the semantics but also help
> > avoid yet-another-entry point and force us to provide a well define structure
> > for considering code that should not run by pegging it as required or supported
> > for different early x86 code stubs.
> 
> Was I unclear last time?  Xen *will not* be introducing Linux-specifics
> into the HVMLite starting ABI.

This does not have to be "Linux specifics" but rather a light way to enable
a hypervisor to clue in *any* OS of its hypervisor type, guest type, and
custom hypervisor data that can be used to populate needed OS specifics
about the guest. Perhaps Xen's own loader mechanism could be extended just
slightly to become *that* standard, its just right now it doesn't seem to
enable for generalizing this in a very useful way for OSes. Its all
custom stubs.

> Your perceived problem with multiple entry points is not a problem with
> multiple entry points; It is a problem with multiple different paths
> performing the same initialisation.

Its actually more of an issue with the lack of strong general semantics
available for different hypervisors and guest types and requirements for x86's
init path. What you end up with as collateral is multiple entry points, and
these can be sloppy and as you note can perform the same initialisation.
Another issue is the inability to proactively ensure new x86 init code
addresses different x86 requirements (cr4 shadow regression and Kasan still
being broken on Xen are two examples) and it just so happens that the lack of
semantics for the different guest types required to be evaluated is one issue
for x86.

We can do better.

> The Linux entry for PV guests is indeed completely horrible.  I am not
> trying to defend it in the slightest.
> 
> However, the HVMLite entry which is a very short stub that sets up a
> zeropage and hands off to the native start routine is fine.

Its alright, and a huge stepping stone towards good architecture. We
however can do better.

> There is still just routine performing native x86 startup.
> 
> If you still desperately want to avoid multiple entry points, then just
> insist on using grub for the VM.  I expect that that is how most people
> will end up using HVMLite VMs anyway.

Are you saying Grub can do some of this heavy lifting that I am trying
to avoid? If so that'd be great news.

  Luis

[toc] | [prev] | [next] | [standalone]

#1326309 — Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest

On Wed, Feb 3, 2016 at 3:59 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
>> If you still desperately want to avoid multiple entry points, then just
>> insist on using grub for the VM.  I expect that that is how most people
>> will end up using HVMLite VMs anyway.
>
> Are you saying Grub can do some of this heavy lifting that I am trying
> to avoid? If so that'd be great news.

Come to think of it, it would still leave the same semantic gap issue,
which is one of the reasons I'm pushing for a solution for that now.

 Luis

[toc] | [prev] | [next] | [standalone]

#1326323 — Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest

On 03/02/2016 23:59, Luis R. Rodriguez wrote:
> On Wed, Feb 03, 2016 at 08:52:50PM +0000, Andrew Cooper wrote:
>> On 03/02/16 18:55, Luis R. Rodriguez wrote:
>>> We add new hypervisor type to close the semantic gap for hypervisor types, and
>>> much like subarch enable also a subarch_data to let you pass and use your
>>> hvmlite_start_info. This would not only help with the semantics but also help
>>> avoid yet-another-entry point and force us to provide a well define structure
>>> for considering code that should not run by pegging it as required or supported
>>> for different early x86 code stubs.
>> Was I unclear last time?  Xen *will not* be introducing Linux-specifics
>> into the HVMLite starting ABI.
> This does not have to be "Linux specifics" but rather a light way to enable
> a hypervisor to clue in *any* OS of its hypervisor type, guest type, and
> custom hypervisor data that can be used to populate needed OS specifics
> about the guest. Perhaps Xen's own loader mechanism could be extended just
> slightly to become *that* standard, its just right now it doesn't seem to
> enable for generalizing this in a very useful way for OSes. Its all
> custom stubs.

There are already standard x86 ways of doing this, via the hypervisor
cpuid bits.  Xen presents itself normally in this regard, as do all the
other hypervisors.

It is completely backwards to expect a hypervisor (or toolstack in our
case) to deliberately prod what it suspects might be a Linux binary in a
way which it things a Linux binary might like to be prodded.

>
>> Your perceived problem with multiple entry points is not a problem with
>> multiple entry points; It is a problem with multiple different paths
>> performing the same initialisation.
> Its actually more of an issue with the lack of strong general semantics
> available for different hypervisors and guest types and requirements for x86's
> init path. What you end up with as collateral is multiple entry points, and
> these can be sloppy and as you note can perform the same initialisation.
> Another issue is the inability to proactively ensure new x86 init code
> addresses different x86 requirements (cr4 shadow regression and Kasan still
> being broken on Xen are two examples) and it just so happens that the lack of
> semantics for the different guest types required to be evaluated is one issue
> for x86.
>
> We can do better.

Even with a perfect startup() routine which caters for all runtime
usecases, you cannot avoid having multiple entry stubs to cater for the
different ways the binary might be started.

Unless you are volunteering to write a single stub which can first
evaluate whether it is in 16/32/64bit mode, then create a safe stack to
use, then evaluate how it was started (multiboot, legacy BIOS, EFI,
etc.) and turn all this information into a zeropage.

I don't know that would be possible, but the point is moot as it
definitely wouldn't be maintainable if it were possible.

>
>> The Linux entry for PV guests is indeed completely horrible.  I am not
>> trying to defend it in the slightest.
>>
>> However, the HVMLite entry which is a very short stub that sets up a
>> zeropage and hands off to the native start routine is fine.
> Its alright, and a huge stepping stone towards good architecture. We
> however can do better.

Then we are generally in agreement.  However, at the risk of sounding
like a grump old sod,  take this win first and then work on the next
stepping stone.

Review comments identifying "I am working on implementing a new $X which
will make this area better/easier/more shiny in the future" are fine. 
Review comments complaining that "you haven't used this shiny new $X
which doesn't exist yet" are a waste of time; time which you would be
better spent implementing said $X.

Noone is disagreeing that improvements can be made, but don't try to do
them all at once, or nothing will get done.

>
>> There is still just routine performing native x86 startup.
>>
>> If you still desperately want to avoid multiple entry points, then just
>> insist on using grub for the VM.  I expect that that is how most people
>> will end up using HVMLite VMs anyway.
> Are you saying Grub can do some of this heavy lifting that I am trying
> to avoid? If so that'd be great news.

There are two different ways of running your VM, depending on your usecase.

The more traditional approach of a full OS will want to load a kernel
out of the guests filesystem, according to the guests bootloader
configuration.  At this point it doesn't matter for Linux as it will be
booted by some already-existing bootloader which already knows how to do
the job.

The more container/unikernel oriented approach is to boot an image
directly from dom0, skipping the middle layers of firmware and
filesystems.  For this to work, Linux needs to be able to be started via
the hypervisor ABI of choice, which in this case is something which
looks very much like multiboot.

~Andrew

[toc] | [prev] | [next] | [standalone]

#1327333 — Re: [Xen-devel] [PATCH v2 02/11] xen/hvmlite: Bootstrap HVMlite guest

On Thu, Feb 04, 2016 at 12:51:38AM +0000, Andrew Cooper wrote:
> On 03/02/2016 23:59, Luis R. Rodriguez wrote:
> > On Wed, Feb 03, 2016 at 08:52:50PM +0000, Andrew Cooper wrote:
> >> On 03/02/16 18:55, Luis R. Rodriguez wrote:
> >>> We add new hypervisor type to close the semantic gap for hypervisor types, and
> >>> much like subarch enable also a subarch_data to let you pass and use your
> >>> hvmlite_start_info. This would not only help with the semantics but also help
> >>> avoid yet-another-entry point and force us to provide a well define structure
> >>> for considering code that should not run by pegging it as required or supported
> >>> for different early x86 code stubs.
> >> Was I unclear last time?  Xen *will not* be introducing Linux-specifics
> >> into the HVMLite starting ABI.
> > This does not have to be "Linux specifics" but rather a light way to enable
> > a hypervisor to clue in *any* OS of its hypervisor type, guest type, and
> > custom hypervisor data that can be used to populate needed OS specifics
> > about the guest. Perhaps Xen's own loader mechanism could be extended just
> > slightly to become *that* standard, its just right now it doesn't seem to
> > enable for generalizing this in a very useful way for OSes. Its all
> > custom stubs.
> 
> There are already standard x86 ways of doing this, via the hypervisor
> cpuid bits.  Xen presents itself normally in this regard, as do all the
> other hypervisors.

I don't think this is availably early in asm boot? Its why I think the
zero page is convenient. The boot loader should in theory know these
things, as well as if its in 32-bit, 64-bit, etc.

> It is completely backwards to expect a hypervisor (or toolstack in our
> case) to deliberately prod what it suspects might be a Linux binary in a
> way which it things a Linux binary might like to be prodded.

Perhaps prodding tons of info seems ludicrous, however prodding at least a
loader type and custom data pointer to interpret that so that then your stub
can interpret seems sensible for many reasons and I don't think prodding two
things is much to ask for, given the possible gains on clean architecture.
Its why I am suggesting perhaps this should just be standardized.

We need flexibility on both sides.

> >> Your perceived problem with multiple entry points is not a problem with
> >> multiple entry points; It is a problem with multiple different paths
> >> performing the same initialisation.
> > Its actually more of an issue with the lack of strong general semantics
> > available for different hypervisors and guest types and requirements for x86's
> > init path. What you end up with as collateral is multiple entry points, and
> > these can be sloppy and as you note can perform the same initialisation.
> > Another issue is the inability to proactively ensure new x86 init code
> > addresses different x86 requirements (cr4 shadow regression and Kasan still
> > being broken on Xen are two examples) and it just so happens that the lack of
> > semantics for the different guest types required to be evaluated is one issue
> > for x86.
> >
> > We can do better.
> 
> Even with a perfect startup() routine which caters for all runtime
> usecases, you cannot avoid having multiple entry stubs to cater for the
> different ways the binary might be started.
> 
> Unless you are volunteering to write a single stub which can first
> evaluate whether it is in 16/32/64bit mode, then create a safe stack to
> use, then evaluate how it was started (multiboot, legacy BIOS, EFI,
> etc.) and turn all this information into a zeropage.
> 
> I don't know that would be possible, but the point is moot as it
> definitely wouldn't be maintainable if it were possible.

I think some folks have hope at least some of it might be. I can't do this,
otherwise I would have done it already. Given my review of the commit logs on
different entry points, and code I do think its sensible to desire this to help
with semantics on startup and this should in turn help duplication, bugs, but I
obviously do not doubt its difficulty.

Its at least sensible in my mind to strive towards the best possible semantics
and code sharing from x86-64 bit onwards and if I can help with that I'll do
what I can.

> >> The Linux entry for PV guests is indeed completely horrible.  I am not
> >> trying to defend it in the slightest.
> >>
> >> However, the HVMLite entry which is a very short stub that sets up a
> >> zeropage and hands off to the native start routine is fine.
> > Its alright, and a huge stepping stone towards good architecture. We
> > however can do better.
> 
> Then we are generally in agreement.  However, at the risk of sounding
> like a grump old sod,  take this win first and then work on the next
> stepping stone.

I think this is fair.

> Review comments identifying "I am working on implementing a new $X which
> will make this area better/easier/more shiny in the future" are fine. 
> Review comments complaining that "you haven't used this shiny new $X
> which doesn't exist yet" are a waste of time; time which you would be
> better spent implementing said $X.
> 
> No one is disagreeing that improvements can be made, but don't try to do
> them all at once, or nothing will get done.

Its a fair point, the only contending issue here is the use of
paravirt_enabled() and how I'm changing this to paravirt_legacy(),
I think that's it. Other than we can coordinate on both fronts
to later help clean things up further.

> >> There is still just routine performing native x86 startup.
> >>
> >> If you still desperately want to avoid multiple entry points, then just
> >> insist on using grub for the VM.  I expect that that is how most people
> >> will end up using HVMLite VMs anyway.
> > Are you saying Grub can do some of this heavy lifting that I am trying
> > to avoid? If so that'd be great news.
> 
> There are two different ways of running your VM, depending on your usecase.
> 
> The more traditional approach of a full OS will want to load a kernel
> out of the guests filesystem, according to the guests bootloader
> configuration.  At this point it doesn't matter for Linux as it will be
> booted by some already-existing bootloader which already knows how to do
> the job.

The grub instance would be on the guest filesystem right?  So grub would know
it got kicked by Xen, and grub could then prod what we think is right? In
other words you pass on the custom xen struct to grub and then grub does
the zero page filling.

> The more container/unikernel oriented approach is to boot an image
> directly from dom0, skipping the middle layers of firmware and
> filesystems.  For this to work, Linux needs to be able to be started via
> the hypervisor ABI of choice, which in this case is something which
> looks very much like multiboot.

I see thanks. And the hypervisor ABI in no way would ever consider an option to
let OSes have 2 bits of info set, the hypervisor guest type and a pointer to
custom data structure for the guest type, in order to help with a cleaner
startup on OSes?

  Luis

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web