[PATCH 0/3] x86/xen: support booting PVH guest via standard boot path

[PATCH 0/3] x86/xen: support booting PVH guest via standard boot path

[Juergen Gross]

Booting a Xen PVH guest requires a special boot entry as it is
mandatory to setup some Xen-specific interfaces rather early. When grub
or OVMF are used as boot loaders, however, those will fill the boot
parameters in zeropage and there is no longer a need to do something
PVH specific in the early boot path.

This patch series adds support for that scenario by identifying PVH
environment and doing the required init steps via Xen hooks instead of
using a dedicated boot entry.

The dedicated entry is still needed for support of Dom0 running in PVH
mode as in this case there is no grub or OVMF involved for filling in
the boot parameters.

Juergen Gross (3):
  x86/acpi: add test for ACPI_FADT_NO_VGA
  x86: add guest_late_init hook to hypervisor_x86 structure
  x86/xen: use guest_late_init to detect Xen PVH guest

 arch/x86/include/asm/hypervisor.h | 11 +++++++++++
 arch/x86/include/asm/kvm_para.h   |  2 --
 arch/x86/include/asm/x86_init.h   |  1 +
 arch/x86/kernel/acpi/boot.c       |  5 +++++
 arch/x86/kernel/kvm.c             |  3 ++-
 arch/x86/kernel/setup.c           |  2 +-
 arch/x86/xen/enlighten_hvm.c      | 24 ++++++++++++++++++++++--
 arch/x86/xen/enlighten_pvh.c      |  9 ---------
 8 files changed, 42 insertions(+), 15 deletions(-)

-- 
2.12.3

[PATCH 1/3] x86/acpi: add test for ACPI_FADT_NO_VGA

[Juergen Gross]

Add a test for ACPI_FADT_NO_VGA when scanning the FADT and set the new
flag x86_platform.legacy.no_vga accordingly.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/include/asm/x86_init.h | 1 +
 arch/x86/kernel/acpi/boot.c     | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 8a1ebf9540dd..75a60fa52a8a 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -195,6 +195,7 @@ enum x86_legacy_i8042_state {
 struct x86_legacy_features {
 	enum x86_legacy_i8042_state i8042;
 	int rtc;
+	int no_vga;
 	int reserve_bios_regions;
 	struct x86_legacy_devices devices;
 };
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 079535e53e2a..ef9e02e614d0 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -961,6 +961,11 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
 		x86_platform.legacy.rtc = 0;
 	}
 
+	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_VGA) {
+		pr_debug("ACPI: probing for VGA not safe\n");
+		x86_platform.legacy.no_vga = 1;
+	}
+
 #ifdef CONFIG_X86_PM_TIMER
 	/* detect the location of the ACPI PM Timer */
 	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) {
-- 
2.12.3

[PATCH 1/3] x86/acpi: add test for ACPI_FADT_NO_VGA

[Juergen Gross]

Add a test for ACPI_FADT_NO_VGA when scanning the FADT and set the new
flag x86_platform.legacy.no_vga accordingly.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/include/asm/x86_init.h | 1 +
 arch/x86/kernel/acpi/boot.c     | 5 +++++
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 8a1ebf9540dd..75a60fa52a8a 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -195,6 +195,7 @@ enum x86_legacy_i8042_state {
 struct x86_legacy_features {
 	enum x86_legacy_i8042_state i8042;
 	int rtc;
+	int no_vga;
 	int reserve_bios_regions;
 	struct x86_legacy_devices devices;
 };
diff --git a/arch/x86/kernel/acpi/boot.c b/arch/x86/kernel/acpi/boot.c
index 079535e53e2a..ef9e02e614d0 100644
--- a/arch/x86/kernel/acpi/boot.c
+++ b/arch/x86/kernel/acpi/boot.c
@@ -961,6 +961,11 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table)
 		x86_platform.legacy.rtc = 0;
 	}
 
+	if (acpi_gbl_FADT.boot_flags & ACPI_FADT_NO_VGA) {
+		pr_debug("ACPI: probing for VGA not safe\n");
+		x86_platform.legacy.no_vga = 1;
+	}
+
 #ifdef CONFIG_X86_PM_TIMER
 	/* detect the location of the ACPI PM Timer */
 	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID) {
-- 
2.12.3


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

[PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Juergen Gross]

Add a new guest_late_init hook to the hypervisor_x86 structure. It
will replace the current kvm_guest_init() call which is changed to
make use of the new hook.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/include/asm/hypervisor.h | 11 +++++++++++
 arch/x86/include/asm/kvm_para.h   |  2 --
 arch/x86/kernel/kvm.c             |  3 ++-
 arch/x86/kernel/setup.c           |  2 +-
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 0ead9dbb9130..37320687b8cb 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -38,6 +38,9 @@ struct hypervisor_x86 {
 	/* Platform setup (run once per boot) */
 	void		(*init_platform)(void);
 
+	/* Guest late init */
+	void		(*guest_late_init)(void);
+
 	/* X2APIC detection (run once per boot) */
 	bool		(*x2apic_available)(void);
 
@@ -66,9 +69,17 @@ static inline void hypervisor_init_mem_mapping(void)
 	if (x86_hyper && x86_hyper->init_mem_mapping)
 		x86_hyper->init_mem_mapping();
 }
+
+static inline void hypervisor_guest_late_init(void)
+{
+	if (x86_hyper && x86_hyper->guest_late_init)
+		x86_hyper->guest_late_init();
+}
+
 #else
 static inline void init_hypervisor_platform(void) { }
 static inline bool hypervisor_x2apic_available(void) { return false; }
 static inline void hypervisor_init_mem_mapping(void) { }
+static inline void hypervisor_guest_late_init(void) { }
 #endif /* CONFIG_HYPERVISOR_GUEST */
 #endif /* _ASM_X86_HYPERVISOR_H */
diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index c373e44049b1..7b407dda2bd7 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -88,7 +88,6 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
 #ifdef CONFIG_KVM_GUEST
 bool kvm_para_available(void);
 unsigned int kvm_arch_para_features(void);
-void __init kvm_guest_init(void);
 void kvm_async_pf_task_wait(u32 token, int interrupt_kernel);
 void kvm_async_pf_task_wake(u32 token);
 u32 kvm_read_and_reset_pf_reason(void);
@@ -103,7 +102,6 @@ static inline void kvm_spinlock_init(void)
 #endif /* CONFIG_PARAVIRT_SPINLOCKS */
 
 #else /* CONFIG_KVM_GUEST */
-#define kvm_guest_init() do {} while (0)
 #define kvm_async_pf_task_wait(T, I) do {} while(0)
 #define kvm_async_pf_task_wake(T) do {} while(0)
 
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 8bb9594d0761..d331b5060aa9 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -465,7 +465,7 @@ static void __init kvm_apf_trap_init(void)
 	update_intr_gate(X86_TRAP_PF, async_page_fault);
 }
 
-void __init kvm_guest_init(void)
+static void __init kvm_guest_init(void)
 {
 	int i;
 
@@ -548,6 +548,7 @@ const struct hypervisor_x86 x86_hyper_kvm __refconst = {
 	.name			= "KVM",
 	.detect			= kvm_detect,
 	.x2apic_available	= kvm_para_available,
+	.guest_late_init	= kvm_guest_init,
 };
 EXPORT_SYMBOL_GPL(x86_hyper_kvm);
 
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 0957dd73d127..578569481d87 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1294,7 +1294,7 @@ void __init setup_arch(char **cmdline_p)
 
 	io_apic_init_mappings();
 
-	kvm_guest_init();
+	hypervisor_guest_late_init();
 
 	e820__reserve_resources();
 	e820__register_nosave_regions(max_low_pfn);
-- 
2.12.3

[PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Juergen Gross]

Add a new guest_late_init hook to the hypervisor_x86 structure. It
will replace the current kvm_guest_init() call which is changed to
make use of the new hook.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/include/asm/hypervisor.h | 11 +++++++++++
 arch/x86/include/asm/kvm_para.h   |  2 --
 arch/x86/kernel/kvm.c             |  3 ++-
 arch/x86/kernel/setup.c           |  2 +-
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
index 0ead9dbb9130..37320687b8cb 100644
--- a/arch/x86/include/asm/hypervisor.h
+++ b/arch/x86/include/asm/hypervisor.h
@@ -38,6 +38,9 @@ struct hypervisor_x86 {
 	/* Platform setup (run once per boot) */
 	void		(*init_platform)(void);
 
+	/* Guest late init */
+	void		(*guest_late_init)(void);
+
 	/* X2APIC detection (run once per boot) */
 	bool		(*x2apic_available)(void);
 
@@ -66,9 +69,17 @@ static inline void hypervisor_init_mem_mapping(void)
 	if (x86_hyper && x86_hyper->init_mem_mapping)
 		x86_hyper->init_mem_mapping();
 }
+
+static inline void hypervisor_guest_late_init(void)
+{
+	if (x86_hyper && x86_hyper->guest_late_init)
+		x86_hyper->guest_late_init();
+}
+
 #else
 static inline void init_hypervisor_platform(void) { }
 static inline bool hypervisor_x2apic_available(void) { return false; }
 static inline void hypervisor_init_mem_mapping(void) { }
+static inline void hypervisor_guest_late_init(void) { }
 #endif /* CONFIG_HYPERVISOR_GUEST */
 #endif /* _ASM_X86_HYPERVISOR_H */
diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
index c373e44049b1..7b407dda2bd7 100644
--- a/arch/x86/include/asm/kvm_para.h
+++ b/arch/x86/include/asm/kvm_para.h
@@ -88,7 +88,6 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
 #ifdef CONFIG_KVM_GUEST
 bool kvm_para_available(void);
 unsigned int kvm_arch_para_features(void);
-void __init kvm_guest_init(void);
 void kvm_async_pf_task_wait(u32 token, int interrupt_kernel);
 void kvm_async_pf_task_wake(u32 token);
 u32 kvm_read_and_reset_pf_reason(void);
@@ -103,7 +102,6 @@ static inline void kvm_spinlock_init(void)
 #endif /* CONFIG_PARAVIRT_SPINLOCKS */
 
 #else /* CONFIG_KVM_GUEST */
-#define kvm_guest_init() do {} while (0)
 #define kvm_async_pf_task_wait(T, I) do {} while(0)
 #define kvm_async_pf_task_wake(T) do {} while(0)
 
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 8bb9594d0761..d331b5060aa9 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -465,7 +465,7 @@ static void __init kvm_apf_trap_init(void)
 	update_intr_gate(X86_TRAP_PF, async_page_fault);
 }
 
-void __init kvm_guest_init(void)
+static void __init kvm_guest_init(void)
 {
 	int i;
 
@@ -548,6 +548,7 @@ const struct hypervisor_x86 x86_hyper_kvm __refconst = {
 	.name			= "KVM",
 	.detect			= kvm_detect,
 	.x2apic_available	= kvm_para_available,
+	.guest_late_init	= kvm_guest_init,
 };
 EXPORT_SYMBOL_GPL(x86_hyper_kvm);
 
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 0957dd73d127..578569481d87 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1294,7 +1294,7 @@ void __init setup_arch(char **cmdline_p)
 
 	io_apic_init_mappings();
 
-	kvm_guest_init();
+	hypervisor_guest_late_init();
 
 	e820__reserve_resources();
 	e820__register_nosave_regions(max_low_pfn);
-- 
2.12.3


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

[PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

In case we are booted via the default boot entry by a generic loader
like grub or OVMF it is necessary to distinguish between a HVM guest
with a device model supporting legacy devices and a PVH guest without
device model.

PVH guests will always have x86_platform.legacy.no_vga set and
x86_platform.legacy.rtc cleared, while both won't be true for HVM
guests.

Test for both conditions in the guest_late_init hook and set xen_pvh
to true if they are met.

Move some of the early PVH initializations to the new hook in order
to avoid duplicated code.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/enlighten_hvm.c | 24 ++++++++++++++++++++++--
 arch/x86/xen/enlighten_pvh.c |  9 ---------
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index de503c225ae1..d7d68a39073a 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -1,3 +1,4 @@
+#include <linux/acpi.h>
 #include <linux/cpu.h>
 #include <linux/kexec.h>
 #include <linux/memblock.h>
@@ -188,8 +189,6 @@ static void __init xen_hvm_guest_init(void)
 	xen_hvm_init_time_ops();
 	xen_hvm_init_mmu_ops();
 
-	if (xen_pvh_domain())
-		machine_ops.emergency_restart = xen_emergency_restart;
 #ifdef CONFIG_KEXEC_CORE
 	machine_ops.shutdown = xen_hvm_shutdown;
 	machine_ops.crash_shutdown = xen_hvm_crash_shutdown;
@@ -226,6 +225,26 @@ static uint32_t __init xen_platform_hvm(void)
 	return xen_cpuid_base();
 }
 
+static __init void xen_hvm_guest_late_init(void)
+{
+#ifdef CONFIG_XEN_PVH
+	/* Test for PVH domain (PVH boot path taken overrides ACPI flags). */
+	if (!xen_pvh &&
+	    (x86_platform.legacy.rtc || !x86_platform.legacy.no_vga))
+		return;
+
+	/* PVH detected. */
+	xen_pvh = true;
+
+	/* Make sure we don't fall back to (default) ACPI_IRQ_MODEL_PIC. */
+	if (!nr_ioapics && acpi_irq_model == ACPI_IRQ_MODEL_PIC)
+		acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;
+
+	machine_ops.emergency_restart = xen_emergency_restart;
+	pv_info.name = "Xen PVH";
+#endif
+}
+
 const struct hypervisor_x86 x86_hyper_xen_hvm = {
 	.name                   = "Xen HVM",
 	.detect                 = xen_platform_hvm,
@@ -233,5 +252,6 @@ const struct hypervisor_x86 x86_hyper_xen_hvm = {
 	.pin_vcpu               = xen_pin_vcpu,
 	.x2apic_available       = xen_x2apic_para_available,
 	.init_mem_mapping	= xen_hvm_init_mem_mapping,
+	.guest_late_init	= xen_hvm_guest_late_init,
 };
 EXPORT_SYMBOL(x86_hyper_xen_hvm);
diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index 7bd3ee08393e..436c4f003e17 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -25,13 +25,6 @@ struct boot_params pvh_bootparams __attribute__((section(".data")));
 struct hvm_start_info pvh_start_info;
 unsigned int pvh_start_info_sz = sizeof(pvh_start_info);
 
-static void xen_pvh_arch_setup(void)
-{
-	/* Make sure we don't fall back to (default) ACPI_IRQ_MODEL_PIC. */
-	if (nr_ioapics == 0)
-		acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;
-}
-
 static void __init init_pvh_bootparams(void)
 {
 	struct xen_memory_map memmap;
@@ -102,6 +95,4 @@ void __init xen_prepare_pvh(void)
 	wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
 
 	init_pvh_bootparams();
-
-	x86_init.oem.arch_setup = xen_pvh_arch_setup;
 }
-- 
2.12.3

[PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

In case we are booted via the default boot entry by a generic loader
like grub or OVMF it is necessary to distinguish between a HVM guest
with a device model supporting legacy devices and a PVH guest without
device model.

PVH guests will always have x86_platform.legacy.no_vga set and
x86_platform.legacy.rtc cleared, while both won't be true for HVM
guests.

Test for both conditions in the guest_late_init hook and set xen_pvh
to true if they are met.

Move some of the early PVH initializations to the new hook in order
to avoid duplicated code.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/xen/enlighten_hvm.c | 24 ++++++++++++++++++++++--
 arch/x86/xen/enlighten_pvh.c |  9 ---------
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/arch/x86/xen/enlighten_hvm.c b/arch/x86/xen/enlighten_hvm.c
index de503c225ae1..d7d68a39073a 100644
--- a/arch/x86/xen/enlighten_hvm.c
+++ b/arch/x86/xen/enlighten_hvm.c
@@ -1,3 +1,4 @@
+#include <linux/acpi.h>
 #include <linux/cpu.h>
 #include <linux/kexec.h>
 #include <linux/memblock.h>
@@ -188,8 +189,6 @@ static void __init xen_hvm_guest_init(void)
 	xen_hvm_init_time_ops();
 	xen_hvm_init_mmu_ops();
 
-	if (xen_pvh_domain())
-		machine_ops.emergency_restart = xen_emergency_restart;
 #ifdef CONFIG_KEXEC_CORE
 	machine_ops.shutdown = xen_hvm_shutdown;
 	machine_ops.crash_shutdown = xen_hvm_crash_shutdown;
@@ -226,6 +225,26 @@ static uint32_t __init xen_platform_hvm(void)
 	return xen_cpuid_base();
 }
 
+static __init void xen_hvm_guest_late_init(void)
+{
+#ifdef CONFIG_XEN_PVH
+	/* Test for PVH domain (PVH boot path taken overrides ACPI flags). */
+	if (!xen_pvh &&
+	    (x86_platform.legacy.rtc || !x86_platform.legacy.no_vga))
+		return;
+
+	/* PVH detected. */
+	xen_pvh = true;
+
+	/* Make sure we don't fall back to (default) ACPI_IRQ_MODEL_PIC. */
+	if (!nr_ioapics && acpi_irq_model == ACPI_IRQ_MODEL_PIC)
+		acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;
+
+	machine_ops.emergency_restart = xen_emergency_restart;
+	pv_info.name = "Xen PVH";
+#endif
+}
+
 const struct hypervisor_x86 x86_hyper_xen_hvm = {
 	.name                   = "Xen HVM",
 	.detect                 = xen_platform_hvm,
@@ -233,5 +252,6 @@ const struct hypervisor_x86 x86_hyper_xen_hvm = {
 	.pin_vcpu               = xen_pin_vcpu,
 	.x2apic_available       = xen_x2apic_para_available,
 	.init_mem_mapping	= xen_hvm_init_mem_mapping,
+	.guest_late_init	= xen_hvm_guest_late_init,
 };
 EXPORT_SYMBOL(x86_hyper_xen_hvm);
diff --git a/arch/x86/xen/enlighten_pvh.c b/arch/x86/xen/enlighten_pvh.c
index 7bd3ee08393e..436c4f003e17 100644
--- a/arch/x86/xen/enlighten_pvh.c
+++ b/arch/x86/xen/enlighten_pvh.c
@@ -25,13 +25,6 @@ struct boot_params pvh_bootparams __attribute__((section(".data")));
 struct hvm_start_info pvh_start_info;
 unsigned int pvh_start_info_sz = sizeof(pvh_start_info);
 
-static void xen_pvh_arch_setup(void)
-{
-	/* Make sure we don't fall back to (default) ACPI_IRQ_MODEL_PIC. */
-	if (nr_ioapics == 0)
-		acpi_irq_model = ACPI_IRQ_MODEL_PLATFORM;
-}
-
 static void __init init_pvh_bootparams(void)
 {
 	struct xen_memory_map memmap;
@@ -102,6 +95,4 @@ void __init xen_prepare_pvh(void)
 	wrmsr_safe(msr, (u32)pfn, (u32)(pfn >> 32));
 
 	init_pvh_bootparams();
-
-	x86_init.oem.arch_setup = xen_pvh_arch_setup;
 }
-- 
2.12.3


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

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Ingo Molnar]

* Juergen Gross <jgross@suse.com> wrote:

> Add a new guest_late_init hook to the hypervisor_x86 structure. It
> will replace the current kvm_guest_init() call which is changed to
> make use of the new hook.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/include/asm/hypervisor.h | 11 +++++++++++
>  arch/x86/include/asm/kvm_para.h   |  2 --
>  arch/x86/kernel/kvm.c             |  3 ++-
>  arch/x86/kernel/setup.c           |  2 +-
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
> index 0ead9dbb9130..37320687b8cb 100644
> --- a/arch/x86/include/asm/hypervisor.h
> +++ b/arch/x86/include/asm/hypervisor.h
> @@ -38,6 +38,9 @@ struct hypervisor_x86 {
>  	/* Platform setup (run once per boot) */
>  	void		(*init_platform)(void);
>  
> +	/* Guest late init */
> +	void		(*guest_late_init)(void);
> +
>  	/* X2APIC detection (run once per boot) */
>  	bool		(*x2apic_available)(void);
>  
> @@ -66,9 +69,17 @@ static inline void hypervisor_init_mem_mapping(void)
>  	if (x86_hyper && x86_hyper->init_mem_mapping)
>  		x86_hyper->init_mem_mapping();
>  }
> +
> +static inline void hypervisor_guest_late_init(void)
> +{
> +	if (x86_hyper && x86_hyper->guest_late_init)
> +		x86_hyper->guest_late_init();
> +}
> +
>  #else
>  static inline void init_hypervisor_platform(void) { }
>  static inline bool hypervisor_x2apic_available(void) { return false; }
>  static inline void hypervisor_init_mem_mapping(void) { }
> +static inline void hypervisor_guest_late_init(void) { }
>  #endif /* CONFIG_HYPERVISOR_GUEST */
>  #endif /* _ASM_X86_HYPERVISOR_H */
> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
> index c373e44049b1..7b407dda2bd7 100644
> --- a/arch/x86/include/asm/kvm_para.h
> +++ b/arch/x86/include/asm/kvm_para.h
> @@ -88,7 +88,6 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
>  #ifdef CONFIG_KVM_GUEST
>  bool kvm_para_available(void);
>  unsigned int kvm_arch_para_features(void);
> -void __init kvm_guest_init(void);
>  void kvm_async_pf_task_wait(u32 token, int interrupt_kernel);
>  void kvm_async_pf_task_wake(u32 token);
>  u32 kvm_read_and_reset_pf_reason(void);
> @@ -103,7 +102,6 @@ static inline void kvm_spinlock_init(void)
>  #endif /* CONFIG_PARAVIRT_SPINLOCKS */
>  
>  #else /* CONFIG_KVM_GUEST */
> -#define kvm_guest_init() do {} while (0)
>  #define kvm_async_pf_task_wait(T, I) do {} while(0)
>  #define kvm_async_pf_task_wake(T) do {} while(0)
>  
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8bb9594d0761..d331b5060aa9 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -465,7 +465,7 @@ static void __init kvm_apf_trap_init(void)
>  	update_intr_gate(X86_TRAP_PF, async_page_fault);
>  }
>  
> -void __init kvm_guest_init(void)
> +static void __init kvm_guest_init(void)
>  {
>  	int i;
>  
> @@ -548,6 +548,7 @@ const struct hypervisor_x86 x86_hyper_kvm __refconst = {
>  	.name			= "KVM",
>  	.detect			= kvm_detect,
>  	.x2apic_available	= kvm_para_available,
> +	.guest_late_init	= kvm_guest_init,
>  };
>  EXPORT_SYMBOL_GPL(x86_hyper_kvm);
>  
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 0957dd73d127..578569481d87 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1294,7 +1294,7 @@ void __init setup_arch(char **cmdline_p)
>  
>  	io_apic_init_mappings();
>  
> -	kvm_guest_init();
> +	hypervisor_guest_late_init();

No principal objections, but could we please use a more obvious pattern showing 
that this is a callback, by calling it directly:
	
	x86_hyper->guest_late_init();

Plus add a default empty function (which hypervisors can override). This avoids 
all the hidden conditions and wrappery.

Thanks,

	Ingo

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Ingo Molnar]

* Juergen Gross <jgross@suse.com> wrote:

> Add a new guest_late_init hook to the hypervisor_x86 structure. It
> will replace the current kvm_guest_init() call which is changed to
> make use of the new hook.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/include/asm/hypervisor.h | 11 +++++++++++
>  arch/x86/include/asm/kvm_para.h   |  2 --
>  arch/x86/kernel/kvm.c             |  3 ++-
>  arch/x86/kernel/setup.c           |  2 +-
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
> index 0ead9dbb9130..37320687b8cb 100644
> --- a/arch/x86/include/asm/hypervisor.h
> +++ b/arch/x86/include/asm/hypervisor.h
> @@ -38,6 +38,9 @@ struct hypervisor_x86 {
>  	/* Platform setup (run once per boot) */
>  	void		(*init_platform)(void);
>  
> +	/* Guest late init */
> +	void		(*guest_late_init)(void);
> +
>  	/* X2APIC detection (run once per boot) */
>  	bool		(*x2apic_available)(void);
>  
> @@ -66,9 +69,17 @@ static inline void hypervisor_init_mem_mapping(void)
>  	if (x86_hyper && x86_hyper->init_mem_mapping)
>  		x86_hyper->init_mem_mapping();
>  }
> +
> +static inline void hypervisor_guest_late_init(void)
> +{
> +	if (x86_hyper && x86_hyper->guest_late_init)
> +		x86_hyper->guest_late_init();
> +}
> +
>  #else
>  static inline void init_hypervisor_platform(void) { }
>  static inline bool hypervisor_x2apic_available(void) { return false; }
>  static inline void hypervisor_init_mem_mapping(void) { }
> +static inline void hypervisor_guest_late_init(void) { }
>  #endif /* CONFIG_HYPERVISOR_GUEST */
>  #endif /* _ASM_X86_HYPERVISOR_H */
> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
> index c373e44049b1..7b407dda2bd7 100644
> --- a/arch/x86/include/asm/kvm_para.h
> +++ b/arch/x86/include/asm/kvm_para.h
> @@ -88,7 +88,6 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
>  #ifdef CONFIG_KVM_GUEST
>  bool kvm_para_available(void);
>  unsigned int kvm_arch_para_features(void);
> -void __init kvm_guest_init(void);
>  void kvm_async_pf_task_wait(u32 token, int interrupt_kernel);
>  void kvm_async_pf_task_wake(u32 token);
>  u32 kvm_read_and_reset_pf_reason(void);
> @@ -103,7 +102,6 @@ static inline void kvm_spinlock_init(void)
>  #endif /* CONFIG_PARAVIRT_SPINLOCKS */
>  
>  #else /* CONFIG_KVM_GUEST */
> -#define kvm_guest_init() do {} while (0)
>  #define kvm_async_pf_task_wait(T, I) do {} while(0)
>  #define kvm_async_pf_task_wake(T) do {} while(0)
>  
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8bb9594d0761..d331b5060aa9 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -465,7 +465,7 @@ static void __init kvm_apf_trap_init(void)
>  	update_intr_gate(X86_TRAP_PF, async_page_fault);
>  }
>  
> -void __init kvm_guest_init(void)
> +static void __init kvm_guest_init(void)
>  {
>  	int i;
>  
> @@ -548,6 +548,7 @@ const struct hypervisor_x86 x86_hyper_kvm __refconst = {
>  	.name			= "KVM",
>  	.detect			= kvm_detect,
>  	.x2apic_available	= kvm_para_available,
> +	.guest_late_init	= kvm_guest_init,
>  };
>  EXPORT_SYMBOL_GPL(x86_hyper_kvm);
>  
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 0957dd73d127..578569481d87 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1294,7 +1294,7 @@ void __init setup_arch(char **cmdline_p)
>  
>  	io_apic_init_mappings();
>  
> -	kvm_guest_init();
> +	hypervisor_guest_late_init();

No principal objections, but could we please use a more obvious pattern showing 
that this is a callback, by calling it directly:
	
	x86_hyper->guest_late_init();

Plus add a default empty function (which hypervisors can override). This avoids 
all the hidden conditions and wrappery.

Thanks,

	Ingo

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

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Juergen Gross]

On 08/11/17 10:13, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@suse.com> wrote:
> 
>> Add a new guest_late_init hook to the hypervisor_x86 structure. It
>> will replace the current kvm_guest_init() call which is changed to
>> make use of the new hook.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  arch/x86/include/asm/hypervisor.h | 11 +++++++++++
>>  arch/x86/include/asm/kvm_para.h   |  2 --
>>  arch/x86/kernel/kvm.c             |  3 ++-
>>  arch/x86/kernel/setup.c           |  2 +-
>>  4 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
>> index 0ead9dbb9130..37320687b8cb 100644
>> --- a/arch/x86/include/asm/hypervisor.h
>> +++ b/arch/x86/include/asm/hypervisor.h
>> @@ -38,6 +38,9 @@ struct hypervisor_x86 {
>>  	/* Platform setup (run once per boot) */
>>  	void		(*init_platform)(void);
>>  
>> +	/* Guest late init */
>> +	void		(*guest_late_init)(void);
>> +
>>  	/* X2APIC detection (run once per boot) */
>>  	bool		(*x2apic_available)(void);
>>  
>> @@ -66,9 +69,17 @@ static inline void hypervisor_init_mem_mapping(void)
>>  	if (x86_hyper && x86_hyper->init_mem_mapping)
>>  		x86_hyper->init_mem_mapping();
>>  }
>> +
>> +static inline void hypervisor_guest_late_init(void)
>> +{
>> +	if (x86_hyper && x86_hyper->guest_late_init)
>> +		x86_hyper->guest_late_init();
>> +}
>> +
>>  #else
>>  static inline void init_hypervisor_platform(void) { }
>>  static inline bool hypervisor_x2apic_available(void) { return false; }
>>  static inline void hypervisor_init_mem_mapping(void) { }
>> +static inline void hypervisor_guest_late_init(void) { }
>>  #endif /* CONFIG_HYPERVISOR_GUEST */
>>  #endif /* _ASM_X86_HYPERVISOR_H */
>> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
>> index c373e44049b1..7b407dda2bd7 100644
>> --- a/arch/x86/include/asm/kvm_para.h
>> +++ b/arch/x86/include/asm/kvm_para.h
>> @@ -88,7 +88,6 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
>>  #ifdef CONFIG_KVM_GUEST
>>  bool kvm_para_available(void);
>>  unsigned int kvm_arch_para_features(void);
>> -void __init kvm_guest_init(void);
>>  void kvm_async_pf_task_wait(u32 token, int interrupt_kernel);
>>  void kvm_async_pf_task_wake(u32 token);
>>  u32 kvm_read_and_reset_pf_reason(void);
>> @@ -103,7 +102,6 @@ static inline void kvm_spinlock_init(void)
>>  #endif /* CONFIG_PARAVIRT_SPINLOCKS */
>>  
>>  #else /* CONFIG_KVM_GUEST */
>> -#define kvm_guest_init() do {} while (0)
>>  #define kvm_async_pf_task_wait(T, I) do {} while(0)
>>  #define kvm_async_pf_task_wake(T) do {} while(0)
>>  
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index 8bb9594d0761..d331b5060aa9 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -465,7 +465,7 @@ static void __init kvm_apf_trap_init(void)
>>  	update_intr_gate(X86_TRAP_PF, async_page_fault);
>>  }
>>  
>> -void __init kvm_guest_init(void)
>> +static void __init kvm_guest_init(void)
>>  {
>>  	int i;
>>  
>> @@ -548,6 +548,7 @@ const struct hypervisor_x86 x86_hyper_kvm __refconst = {
>>  	.name			= "KVM",
>>  	.detect			= kvm_detect,
>>  	.x2apic_available	= kvm_para_available,
>> +	.guest_late_init	= kvm_guest_init,
>>  };
>>  EXPORT_SYMBOL_GPL(x86_hyper_kvm);
>>  
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index 0957dd73d127..578569481d87 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -1294,7 +1294,7 @@ void __init setup_arch(char **cmdline_p)
>>  
>>  	io_apic_init_mappings();
>>  
>> -	kvm_guest_init();
>> +	hypervisor_guest_late_init();
> 
> No principal objections, but could we please use a more obvious pattern showing 
> that this is a callback, by calling it directly:
> 	
> 	x86_hyper->guest_late_init();
> 
> Plus add a default empty function (which hypervisors can override). This avoids 
> all the hidden conditions and wrappery.

Hmm, x86_hyper is just a pointer being NULL on bare metal. So we would
have to add a pre-filled struct with dummy functions and in case a
hypervisor is detected we'd need to copy all non-NULL pointers of the
hypervisor specific struct to the pre-filled one.

In case there are no objections I can add a patch to modify the current
way x86_hyper is used to the pre-filled variant.


Juergen

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Juergen Gross]

On 08/11/17 10:13, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@suse.com> wrote:
> 
>> Add a new guest_late_init hook to the hypervisor_x86 structure. It
>> will replace the current kvm_guest_init() call which is changed to
>> make use of the new hook.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>  arch/x86/include/asm/hypervisor.h | 11 +++++++++++
>>  arch/x86/include/asm/kvm_para.h   |  2 --
>>  arch/x86/kernel/kvm.c             |  3 ++-
>>  arch/x86/kernel/setup.c           |  2 +-
>>  4 files changed, 14 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
>> index 0ead9dbb9130..37320687b8cb 100644
>> --- a/arch/x86/include/asm/hypervisor.h
>> +++ b/arch/x86/include/asm/hypervisor.h
>> @@ -38,6 +38,9 @@ struct hypervisor_x86 {
>>  	/* Platform setup (run once per boot) */
>>  	void		(*init_platform)(void);
>>  
>> +	/* Guest late init */
>> +	void		(*guest_late_init)(void);
>> +
>>  	/* X2APIC detection (run once per boot) */
>>  	bool		(*x2apic_available)(void);
>>  
>> @@ -66,9 +69,17 @@ static inline void hypervisor_init_mem_mapping(void)
>>  	if (x86_hyper && x86_hyper->init_mem_mapping)
>>  		x86_hyper->init_mem_mapping();
>>  }
>> +
>> +static inline void hypervisor_guest_late_init(void)
>> +{
>> +	if (x86_hyper && x86_hyper->guest_late_init)
>> +		x86_hyper->guest_late_init();
>> +}
>> +
>>  #else
>>  static inline void init_hypervisor_platform(void) { }
>>  static inline bool hypervisor_x2apic_available(void) { return false; }
>>  static inline void hypervisor_init_mem_mapping(void) { }
>> +static inline void hypervisor_guest_late_init(void) { }
>>  #endif /* CONFIG_HYPERVISOR_GUEST */
>>  #endif /* _ASM_X86_HYPERVISOR_H */
>> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
>> index c373e44049b1..7b407dda2bd7 100644
>> --- a/arch/x86/include/asm/kvm_para.h
>> +++ b/arch/x86/include/asm/kvm_para.h
>> @@ -88,7 +88,6 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
>>  #ifdef CONFIG_KVM_GUEST
>>  bool kvm_para_available(void);
>>  unsigned int kvm_arch_para_features(void);
>> -void __init kvm_guest_init(void);
>>  void kvm_async_pf_task_wait(u32 token, int interrupt_kernel);
>>  void kvm_async_pf_task_wake(u32 token);
>>  u32 kvm_read_and_reset_pf_reason(void);
>> @@ -103,7 +102,6 @@ static inline void kvm_spinlock_init(void)
>>  #endif /* CONFIG_PARAVIRT_SPINLOCKS */
>>  
>>  #else /* CONFIG_KVM_GUEST */
>> -#define kvm_guest_init() do {} while (0)
>>  #define kvm_async_pf_task_wait(T, I) do {} while(0)
>>  #define kvm_async_pf_task_wake(T) do {} while(0)
>>  
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index 8bb9594d0761..d331b5060aa9 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -465,7 +465,7 @@ static void __init kvm_apf_trap_init(void)
>>  	update_intr_gate(X86_TRAP_PF, async_page_fault);
>>  }
>>  
>> -void __init kvm_guest_init(void)
>> +static void __init kvm_guest_init(void)
>>  {
>>  	int i;
>>  
>> @@ -548,6 +548,7 @@ const struct hypervisor_x86 x86_hyper_kvm __refconst = {
>>  	.name			= "KVM",
>>  	.detect			= kvm_detect,
>>  	.x2apic_available	= kvm_para_available,
>> +	.guest_late_init	= kvm_guest_init,
>>  };
>>  EXPORT_SYMBOL_GPL(x86_hyper_kvm);
>>  
>> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
>> index 0957dd73d127..578569481d87 100644
>> --- a/arch/x86/kernel/setup.c
>> +++ b/arch/x86/kernel/setup.c
>> @@ -1294,7 +1294,7 @@ void __init setup_arch(char **cmdline_p)
>>  
>>  	io_apic_init_mappings();
>>  
>> -	kvm_guest_init();
>> +	hypervisor_guest_late_init();
> 
> No principal objections, but could we please use a more obvious pattern showing 
> that this is a callback, by calling it directly:
> 	
> 	x86_hyper->guest_late_init();
> 
> Plus add a default empty function (which hypervisors can override). This avoids 
> all the hidden conditions and wrappery.

Hmm, x86_hyper is just a pointer being NULL on bare metal. So we would
have to add a pre-filled struct with dummy functions and in case a
hypervisor is detected we'd need to copy all non-NULL pointers of the
hypervisor specific struct to the pre-filled one.

In case there are no objections I can add a patch to modify the current
way x86_hyper is used to the pre-filled variant.


Juergen


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

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Ingo Molnar]

* Juergen Gross <jgross@suse.com> wrote:

> > Plus add a default empty function (which hypervisors can override). This avoids 
> > all the hidden conditions and wrappery.
> 
> Hmm, x86_hyper is just a pointer being NULL on bare metal. So we would
> have to add a pre-filled struct with dummy functions and in case a
> hypervisor is detected we'd need to copy all non-NULL pointers of the
> hypervisor specific struct to the pre-filled one.

Ok, I missed that detail - but yeah, since this is mostly about boot code,
where readability is king, I still think it would be an overall improvement.

This is the pattern we are trying to use with x86_platform ops for example, and 
doing:

  git grep -w x86_platform arch/x86

gives a pretty clear idea about how it's used - while if it was all within 
wrappers it would be a lot more difficult to discover all the callsites.

Admittedly it's not done totally consistently:

 arch/x86/kernel/apic/probe_32.c:        if (x86_platform.apic_post_init)
 arch/x86/kernel/apic/probe_64.c:        if (x86_platform.apic_post_init)
 arch/x86/kernel/ebda.c: if (!x86_platform.legacy.reserve_bios_regions)
 arch/x86/kernel/platform-quirks.c:      if (x86_platform.set_legacy_features)
 arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc.c:   if (!x86_platform.legacy.rtc)

... but the _idea_ behind it is consistent ;-)

> In case there are no objections I can add a patch to modify the current
> way x86_hyper is used to the pre-filled variant.

Yeah, sounds good to me!

Thanks,

	Ingo

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Ingo Molnar]

* Juergen Gross <jgross@suse.com> wrote:

> > Plus add a default empty function (which hypervisors can override). This avoids 
> > all the hidden conditions and wrappery.
> 
> Hmm, x86_hyper is just a pointer being NULL on bare metal. So we would
> have to add a pre-filled struct with dummy functions and in case a
> hypervisor is detected we'd need to copy all non-NULL pointers of the
> hypervisor specific struct to the pre-filled one.

Ok, I missed that detail - but yeah, since this is mostly about boot code,
where readability is king, I still think it would be an overall improvement.

This is the pattern we are trying to use with x86_platform ops for example, and 
doing:

  git grep -w x86_platform arch/x86

gives a pretty clear idea about how it's used - while if it was all within 
wrappers it would be a lot more difficult to discover all the callsites.

Admittedly it's not done totally consistently:

 arch/x86/kernel/apic/probe_32.c:        if (x86_platform.apic_post_init)
 arch/x86/kernel/apic/probe_64.c:        if (x86_platform.apic_post_init)
 arch/x86/kernel/ebda.c: if (!x86_platform.legacy.reserve_bios_regions)
 arch/x86/kernel/platform-quirks.c:      if (x86_platform.set_legacy_features)
 arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc.c:   if (!x86_platform.legacy.rtc)

... but the _idea_ behind it is consistent ;-)

> In case there are no objections I can add a patch to modify the current
> way x86_hyper is used to the pre-filled variant.

Yeah, sounds good to me!

Thanks,

	Ingo

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

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Juergen Gross]

On 08/11/17 10:40, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@suse.com> wrote:
> 
>>> Plus add a default empty function (which hypervisors can override). This avoids 
>>> all the hidden conditions and wrappery.
>>
>> Hmm, x86_hyper is just a pointer being NULL on bare metal. So we would
>> have to add a pre-filled struct with dummy functions and in case a
>> hypervisor is detected we'd need to copy all non-NULL pointers of the
>> hypervisor specific struct to the pre-filled one.
> 
> Ok, I missed that detail - but yeah, since this is mostly about boot code,
> where readability is king, I still think it would be an overall improvement.
> 
> This is the pattern we are trying to use with x86_platform ops for example, and 
> doing:
> 
>   git grep -w x86_platform arch/x86
> 
> gives a pretty clear idea about how it's used - while if it was all within 
> wrappers it would be a lot more difficult to discover all the callsites.
> 
> Admittedly it's not done totally consistently:
> 
>  arch/x86/kernel/apic/probe_32.c:        if (x86_platform.apic_post_init)
>  arch/x86/kernel/apic/probe_64.c:        if (x86_platform.apic_post_init)
>  arch/x86/kernel/ebda.c: if (!x86_platform.legacy.reserve_bios_regions)
>  arch/x86/kernel/platform-quirks.c:      if (x86_platform.set_legacy_features)
>  arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc.c:   if (!x86_platform.legacy.rtc)
> 
> ... but the _idea_ behind it is consistent ;-)
> 
>> In case there are no objections I can add a patch to modify the current
>> way x86_hyper is used to the pre-filled variant.
> 
> Yeah, sounds good to me!

Okay. With you mentioning x86_platform: should I make x86_hyper a member
of x86_platform (e.g. x86_platform.hyper.) ?

I think this would fit quite nice.


Juergen

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Juergen Gross]

On 08/11/17 10:40, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@suse.com> wrote:
> 
>>> Plus add a default empty function (which hypervisors can override). This avoids 
>>> all the hidden conditions and wrappery.
>>
>> Hmm, x86_hyper is just a pointer being NULL on bare metal. So we would
>> have to add a pre-filled struct with dummy functions and in case a
>> hypervisor is detected we'd need to copy all non-NULL pointers of the
>> hypervisor specific struct to the pre-filled one.
> 
> Ok, I missed that detail - but yeah, since this is mostly about boot code,
> where readability is king, I still think it would be an overall improvement.
> 
> This is the pattern we are trying to use with x86_platform ops for example, and 
> doing:
> 
>   git grep -w x86_platform arch/x86
> 
> gives a pretty clear idea about how it's used - while if it was all within 
> wrappers it would be a lot more difficult to discover all the callsites.
> 
> Admittedly it's not done totally consistently:
> 
>  arch/x86/kernel/apic/probe_32.c:        if (x86_platform.apic_post_init)
>  arch/x86/kernel/apic/probe_64.c:        if (x86_platform.apic_post_init)
>  arch/x86/kernel/ebda.c: if (!x86_platform.legacy.reserve_bios_regions)
>  arch/x86/kernel/platform-quirks.c:      if (x86_platform.set_legacy_features)
>  arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc.c:   if (!x86_platform.legacy.rtc)
> 
> ... but the _idea_ behind it is consistent ;-)
> 
>> In case there are no objections I can add a patch to modify the current
>> way x86_hyper is used to the pre-filled variant.
> 
> Yeah, sounds good to me!

Okay. With you mentioning x86_platform: should I make x86_hyper a member
of x86_platform (e.g. x86_platform.hyper.) ?

I think this would fit quite nice.


Juergen

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

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Paolo Bonzini]

On 08/11/2017 10:07, Juergen Gross wrote:
> Add a new guest_late_init hook to the hypervisor_x86 structure. It
> will replace the current kvm_guest_init() call which is changed to
> make use of the new hook.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

The trivial KVM changes are of course

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> ---
>  arch/x86/include/asm/hypervisor.h | 11 +++++++++++
>  arch/x86/include/asm/kvm_para.h   |  2 --
>  arch/x86/kernel/kvm.c             |  3 ++-
>  arch/x86/kernel/setup.c           |  2 +-
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
> index 0ead9dbb9130..37320687b8cb 100644
> --- a/arch/x86/include/asm/hypervisor.h
> +++ b/arch/x86/include/asm/hypervisor.h
> @@ -38,6 +38,9 @@ struct hypervisor_x86 {
>  	/* Platform setup (run once per boot) */
>  	void		(*init_platform)(void);
>  
> +	/* Guest late init */
> +	void		(*guest_late_init)(void);
> +
>  	/* X2APIC detection (run once per boot) */
>  	bool		(*x2apic_available)(void);
>  
> @@ -66,9 +69,17 @@ static inline void hypervisor_init_mem_mapping(void)
>  	if (x86_hyper && x86_hyper->init_mem_mapping)
>  		x86_hyper->init_mem_mapping();
>  }
> +
> +static inline void hypervisor_guest_late_init(void)
> +{
> +	if (x86_hyper && x86_hyper->guest_late_init)
> +		x86_hyper->guest_late_init();
> +}
> +
>  #else
>  static inline void init_hypervisor_platform(void) { }
>  static inline bool hypervisor_x2apic_available(void) { return false; }
>  static inline void hypervisor_init_mem_mapping(void) { }
> +static inline void hypervisor_guest_late_init(void) { }
>  #endif /* CONFIG_HYPERVISOR_GUEST */
>  #endif /* _ASM_X86_HYPERVISOR_H */
> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
> index c373e44049b1..7b407dda2bd7 100644
> --- a/arch/x86/include/asm/kvm_para.h
> +++ b/arch/x86/include/asm/kvm_para.h
> @@ -88,7 +88,6 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
>  #ifdef CONFIG_KVM_GUEST
>  bool kvm_para_available(void);
>  unsigned int kvm_arch_para_features(void);
> -void __init kvm_guest_init(void);
>  void kvm_async_pf_task_wait(u32 token, int interrupt_kernel);
>  void kvm_async_pf_task_wake(u32 token);
>  u32 kvm_read_and_reset_pf_reason(void);
> @@ -103,7 +102,6 @@ static inline void kvm_spinlock_init(void)
>  #endif /* CONFIG_PARAVIRT_SPINLOCKS */
>  
>  #else /* CONFIG_KVM_GUEST */
> -#define kvm_guest_init() do {} while (0)
>  #define kvm_async_pf_task_wait(T, I) do {} while(0)
>  #define kvm_async_pf_task_wake(T) do {} while(0)
>  
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8bb9594d0761..d331b5060aa9 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -465,7 +465,7 @@ static void __init kvm_apf_trap_init(void)
>  	update_intr_gate(X86_TRAP_PF, async_page_fault);
>  }
>  
> -void __init kvm_guest_init(void)
> +static void __init kvm_guest_init(void)
>  {
>  	int i;
>  
> @@ -548,6 +548,7 @@ const struct hypervisor_x86 x86_hyper_kvm __refconst = {
>  	.name			= "KVM",
>  	.detect			= kvm_detect,
>  	.x2apic_available	= kvm_para_available,
> +	.guest_late_init	= kvm_guest_init,
>  };
>  EXPORT_SYMBOL_GPL(x86_hyper_kvm);
>  
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 0957dd73d127..578569481d87 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1294,7 +1294,7 @@ void __init setup_arch(char **cmdline_p)
>  
>  	io_apic_init_mappings();
>  
> -	kvm_guest_init();
> +	hypervisor_guest_late_init();
>  
>  	e820__reserve_resources();
>  	e820__register_nosave_regions(max_low_pfn);
> 

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Paolo Bonzini]

On 08/11/2017 10:07, Juergen Gross wrote:
> Add a new guest_late_init hook to the hypervisor_x86 structure. It
> will replace the current kvm_guest_init() call which is changed to
> make use of the new hook.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>

The trivial KVM changes are of course

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

> ---
>  arch/x86/include/asm/hypervisor.h | 11 +++++++++++
>  arch/x86/include/asm/kvm_para.h   |  2 --
>  arch/x86/kernel/kvm.c             |  3 ++-
>  arch/x86/kernel/setup.c           |  2 +-
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/hypervisor.h b/arch/x86/include/asm/hypervisor.h
> index 0ead9dbb9130..37320687b8cb 100644
> --- a/arch/x86/include/asm/hypervisor.h
> +++ b/arch/x86/include/asm/hypervisor.h
> @@ -38,6 +38,9 @@ struct hypervisor_x86 {
>  	/* Platform setup (run once per boot) */
>  	void		(*init_platform)(void);
>  
> +	/* Guest late init */
> +	void		(*guest_late_init)(void);
> +
>  	/* X2APIC detection (run once per boot) */
>  	bool		(*x2apic_available)(void);
>  
> @@ -66,9 +69,17 @@ static inline void hypervisor_init_mem_mapping(void)
>  	if (x86_hyper && x86_hyper->init_mem_mapping)
>  		x86_hyper->init_mem_mapping();
>  }
> +
> +static inline void hypervisor_guest_late_init(void)
> +{
> +	if (x86_hyper && x86_hyper->guest_late_init)
> +		x86_hyper->guest_late_init();
> +}
> +
>  #else
>  static inline void init_hypervisor_platform(void) { }
>  static inline bool hypervisor_x2apic_available(void) { return false; }
>  static inline void hypervisor_init_mem_mapping(void) { }
> +static inline void hypervisor_guest_late_init(void) { }
>  #endif /* CONFIG_HYPERVISOR_GUEST */
>  #endif /* _ASM_X86_HYPERVISOR_H */
> diff --git a/arch/x86/include/asm/kvm_para.h b/arch/x86/include/asm/kvm_para.h
> index c373e44049b1..7b407dda2bd7 100644
> --- a/arch/x86/include/asm/kvm_para.h
> +++ b/arch/x86/include/asm/kvm_para.h
> @@ -88,7 +88,6 @@ static inline long kvm_hypercall4(unsigned int nr, unsigned long p1,
>  #ifdef CONFIG_KVM_GUEST
>  bool kvm_para_available(void);
>  unsigned int kvm_arch_para_features(void);
> -void __init kvm_guest_init(void);
>  void kvm_async_pf_task_wait(u32 token, int interrupt_kernel);
>  void kvm_async_pf_task_wake(u32 token);
>  u32 kvm_read_and_reset_pf_reason(void);
> @@ -103,7 +102,6 @@ static inline void kvm_spinlock_init(void)
>  #endif /* CONFIG_PARAVIRT_SPINLOCKS */
>  
>  #else /* CONFIG_KVM_GUEST */
> -#define kvm_guest_init() do {} while (0)
>  #define kvm_async_pf_task_wait(T, I) do {} while(0)
>  #define kvm_async_pf_task_wake(T) do {} while(0)
>  
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 8bb9594d0761..d331b5060aa9 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -465,7 +465,7 @@ static void __init kvm_apf_trap_init(void)
>  	update_intr_gate(X86_TRAP_PF, async_page_fault);
>  }
>  
> -void __init kvm_guest_init(void)
> +static void __init kvm_guest_init(void)
>  {
>  	int i;
>  
> @@ -548,6 +548,7 @@ const struct hypervisor_x86 x86_hyper_kvm __refconst = {
>  	.name			= "KVM",
>  	.detect			= kvm_detect,
>  	.x2apic_available	= kvm_para_available,
> +	.guest_late_init	= kvm_guest_init,
>  };
>  EXPORT_SYMBOL_GPL(x86_hyper_kvm);
>  
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 0957dd73d127..578569481d87 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1294,7 +1294,7 @@ void __init setup_arch(char **cmdline_p)
>  
>  	io_apic_init_mappings();
>  
> -	kvm_guest_init();
> +	hypervisor_guest_late_init();
>  
>  	e820__reserve_resources();
>  	e820__register_nosave_regions(max_low_pfn);
> 


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

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Ingo Molnar]

* Juergen Gross <jgross@suse.com> wrote:

> On 08/11/17 10:40, Ingo Molnar wrote:
> > 
> > * Juergen Gross <jgross@suse.com> wrote:
> > 
> >>> Plus add a default empty function (which hypervisors can override). This avoids 
> >>> all the hidden conditions and wrappery.
> >>
> >> Hmm, x86_hyper is just a pointer being NULL on bare metal. So we would
> >> have to add a pre-filled struct with dummy functions and in case a
> >> hypervisor is detected we'd need to copy all non-NULL pointers of the
> >> hypervisor specific struct to the pre-filled one.
> > 
> > Ok, I missed that detail - but yeah, since this is mostly about boot code,
> > where readability is king, I still think it would be an overall improvement.
> > 
> > This is the pattern we are trying to use with x86_platform ops for example, and 
> > doing:
> > 
> >   git grep -w x86_platform arch/x86
> > 
> > gives a pretty clear idea about how it's used - while if it was all within 
> > wrappers it would be a lot more difficult to discover all the callsites.
> > 
> > Admittedly it's not done totally consistently:
> > 
> >  arch/x86/kernel/apic/probe_32.c:        if (x86_platform.apic_post_init)
> >  arch/x86/kernel/apic/probe_64.c:        if (x86_platform.apic_post_init)
> >  arch/x86/kernel/ebda.c: if (!x86_platform.legacy.reserve_bios_regions)
> >  arch/x86/kernel/platform-quirks.c:      if (x86_platform.set_legacy_features)
> >  arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc.c:   if (!x86_platform.legacy.rtc)
> > 
> > ... but the _idea_ behind it is consistent ;-)
> > 
> >> In case there are no objections I can add a patch to modify the current
> >> way x86_hyper is used to the pre-filled variant.
> > 
> > Yeah, sounds good to me!
> 
> Okay. With you mentioning x86_platform: should I make x86_hyper a member
> of x86_platform (e.g. x86_platform.hyper.) ?
> 
> I think this would fit quite nice.

Yeah, seems like a good idea!

Thanks,

	Ingo

Re: [PATCH 2/3] x86: add guest_late_init hook to hypervisor_x86 structure

[Ingo Molnar]

* Juergen Gross <jgross@suse.com> wrote:

> On 08/11/17 10:40, Ingo Molnar wrote:
> > 
> > * Juergen Gross <jgross@suse.com> wrote:
> > 
> >>> Plus add a default empty function (which hypervisors can override). This avoids 
> >>> all the hidden conditions and wrappery.
> >>
> >> Hmm, x86_hyper is just a pointer being NULL on bare metal. So we would
> >> have to add a pre-filled struct with dummy functions and in case a
> >> hypervisor is detected we'd need to copy all non-NULL pointers of the
> >> hypervisor specific struct to the pre-filled one.
> > 
> > Ok, I missed that detail - but yeah, since this is mostly about boot code,
> > where readability is king, I still think it would be an overall improvement.
> > 
> > This is the pattern we are trying to use with x86_platform ops for example, and 
> > doing:
> > 
> >   git grep -w x86_platform arch/x86
> > 
> > gives a pretty clear idea about how it's used - while if it was all within 
> > wrappers it would be a lot more difficult to discover all the callsites.
> > 
> > Admittedly it's not done totally consistently:
> > 
> >  arch/x86/kernel/apic/probe_32.c:        if (x86_platform.apic_post_init)
> >  arch/x86/kernel/apic/probe_64.c:        if (x86_platform.apic_post_init)
> >  arch/x86/kernel/ebda.c: if (!x86_platform.legacy.reserve_bios_regions)
> >  arch/x86/kernel/platform-quirks.c:      if (x86_platform.set_legacy_features)
> >  arch/x86/platform/intel-mid/device_libs/platform_mrfld_rtc.c:   if (!x86_platform.legacy.rtc)
> > 
> > ... but the _idea_ behind it is consistent ;-)
> > 
> >> In case there are no objections I can add a patch to modify the current
> >> way x86_hyper is used to the pre-filled variant.
> > 
> > Yeah, sounds good to me!
> 
> Okay. With you mentioning x86_platform: should I make x86_hyper a member
> of x86_platform (e.g. x86_platform.hyper.) ?
> 
> I think this would fit quite nice.

Yeah, seems like a good idea!

Thanks,

	Ingo

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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Jan Beulich]

>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
> In case we are booted via the default boot entry by a generic loader
> like grub or OVMF it is necessary to distinguish between a HVM guest
> with a device model supporting legacy devices and a PVH guest without
> device model.
> 
> PVH guests will always have x86_platform.legacy.no_vga set and
> x86_platform.legacy.rtc cleared, while both won't be true for HVM
> guests.
> 
> Test for both conditions in the guest_late_init hook and set xen_pvh
> to true if they are met.

This sounds pretty fragile to me: I can't see a reason why a proper
HVM guest couldn't come without VGA and RTC. That's not possible
today, agreed, but certainly an option down the road if virtualization
follows bare metal's road towards being legacy free.

Jan

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Jan Beulich]

>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
> In case we are booted via the default boot entry by a generic loader
> like grub or OVMF it is necessary to distinguish between a HVM guest
> with a device model supporting legacy devices and a PVH guest without
> device model.
> 
> PVH guests will always have x86_platform.legacy.no_vga set and
> x86_platform.legacy.rtc cleared, while both won't be true for HVM
> guests.
> 
> Test for both conditions in the guest_late_init hook and set xen_pvh
> to true if they are met.

This sounds pretty fragile to me: I can't see a reason why a proper
HVM guest couldn't come without VGA and RTC. That's not possible
today, agreed, but certainly an option down the road if virtualization
follows bare metal's road towards being legacy free.

Jan


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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 12:18, Jan Beulich wrote:
>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>> In case we are booted via the default boot entry by a generic loader
>> like grub or OVMF it is necessary to distinguish between a HVM guest
>> with a device model supporting legacy devices and a PVH guest without
>> device model.
>>
>> PVH guests will always have x86_platform.legacy.no_vga set and
>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>> guests.
>>
>> Test for both conditions in the guest_late_init hook and set xen_pvh
>> to true if they are met.
> 
> This sounds pretty fragile to me: I can't see a reason why a proper
> HVM guest couldn't come without VGA and RTC. That's not possible
> today, agreed, but certainly an option down the road if virtualization
> follows bare metal's road towards being legacy free.

>From guest's perspective: what is the difference between a legacy free
HVM domain and PVH? In the end the need for differentiating is to avoid
access to legacy features in PVH as those would require a device model.


Juergen

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 12:18, Jan Beulich wrote:
>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>> In case we are booted via the default boot entry by a generic loader
>> like grub or OVMF it is necessary to distinguish between a HVM guest
>> with a device model supporting legacy devices and a PVH guest without
>> device model.
>>
>> PVH guests will always have x86_platform.legacy.no_vga set and
>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>> guests.
>>
>> Test for both conditions in the guest_late_init hook and set xen_pvh
>> to true if they are met.
> 
> This sounds pretty fragile to me: I can't see a reason why a proper
> HVM guest couldn't come without VGA and RTC. That's not possible
> today, agreed, but certainly an option down the road if virtualization
> follows bare metal's road towards being legacy free.

From guest's perspective: what is the difference between a legacy free
HVM domain and PVH? In the end the need for differentiating is to avoid
access to legacy features in PVH as those would require a device model.


Juergen


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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Paolo Bonzini]

On 08/11/2017 12:55, Juergen Gross wrote:
> On 08/11/17 12:18, Jan Beulich wrote:
>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>> In case we are booted via the default boot entry by a generic loader
>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>> with a device model supporting legacy devices and a PVH guest without
>>> device model.
>>>
>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>> guests.
>>>
>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>> to true if they are met.
>>
>> This sounds pretty fragile to me: I can't see a reason why a proper
>> HVM guest couldn't come without VGA and RTC. That's not possible
>> today, agreed, but certainly an option down the road if virtualization
>> follows bare metal's road towards being legacy free.
> 
> From guest's perspective: what is the difference between a legacy free
> HVM domain and PVH? In the end the need for differentiating is to avoid
> access to legacy features in PVH as those would require a device model.

My understanding of Xen is very rusty at this point, but I think a
"completely" legacy-free HVM domain will still have a PCI bus and the
Xen platform device on that bus.

A PVH domain just knows how to access the Xen PV features.

Paolo

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Paolo Bonzini]

On 08/11/2017 12:55, Juergen Gross wrote:
> On 08/11/17 12:18, Jan Beulich wrote:
>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>> In case we are booted via the default boot entry by a generic loader
>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>> with a device model supporting legacy devices and a PVH guest without
>>> device model.
>>>
>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>> guests.
>>>
>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>> to true if they are met.
>>
>> This sounds pretty fragile to me: I can't see a reason why a proper
>> HVM guest couldn't come without VGA and RTC. That's not possible
>> today, agreed, but certainly an option down the road if virtualization
>> follows bare metal's road towards being legacy free.
> 
> From guest's perspective: what is the difference between a legacy free
> HVM domain and PVH? In the end the need for differentiating is to avoid
> access to legacy features in PVH as those would require a device model.

My understanding of Xen is very rusty at this point, but I think a
"completely" legacy-free HVM domain will still have a PCI bus and the
Xen platform device on that bus.

A PVH domain just knows how to access the Xen PV features.

Paolo

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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 13:03, Paolo Bonzini wrote:
> On 08/11/2017 12:55, Juergen Gross wrote:
>> On 08/11/17 12:18, Jan Beulich wrote:
>>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>>> In case we are booted via the default boot entry by a generic loader
>>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>>> with a device model supporting legacy devices and a PVH guest without
>>>> device model.
>>>>
>>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>>> guests.
>>>>
>>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>>> to true if they are met.
>>>
>>> This sounds pretty fragile to me: I can't see a reason why a proper
>>> HVM guest couldn't come without VGA and RTC. That's not possible
>>> today, agreed, but certainly an option down the road if virtualization
>>> follows bare metal's road towards being legacy free.
>>
>> From guest's perspective: what is the difference between a legacy free
>> HVM domain and PVH? In the end the need for differentiating is to avoid
>> access to legacy features in PVH as those would require a device model.
> 
> My understanding of Xen is very rusty at this point, but I think a
> "completely" legacy-free HVM domain will still have a PCI bus and the
> Xen platform device on that bus.
> 
> A PVH domain just knows how to access the Xen PV features.

A HVM domain does so, too. Today maybe only partially, but e.g. event
channels work in a HVM domain even without the Xen platform device.
Grant tables can be made working without the platform device, too,
and I'm already preparing a patch to do exactly that.

The only need for the platform device will then be to have an
interface for unplugging emulated boot devices in favor of the pv
devices. And without the platform device we can just skip that
step without doing any harm, as this can happen only for PVH where
we have no need to do the unplug, or for HVM explicitly configured
to work without platform device needing to continue using the
emulated devices as it is doing today in this case.


Juergen

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 13:03, Paolo Bonzini wrote:
> On 08/11/2017 12:55, Juergen Gross wrote:
>> On 08/11/17 12:18, Jan Beulich wrote:
>>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>>> In case we are booted via the default boot entry by a generic loader
>>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>>> with a device model supporting legacy devices and a PVH guest without
>>>> device model.
>>>>
>>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>>> guests.
>>>>
>>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>>> to true if they are met.
>>>
>>> This sounds pretty fragile to me: I can't see a reason why a proper
>>> HVM guest couldn't come without VGA and RTC. That's not possible
>>> today, agreed, but certainly an option down the road if virtualization
>>> follows bare metal's road towards being legacy free.
>>
>> From guest's perspective: what is the difference between a legacy free
>> HVM domain and PVH? In the end the need for differentiating is to avoid
>> access to legacy features in PVH as those would require a device model.
> 
> My understanding of Xen is very rusty at this point, but I think a
> "completely" legacy-free HVM domain will still have a PCI bus and the
> Xen platform device on that bus.
> 
> A PVH domain just knows how to access the Xen PV features.

A HVM domain does so, too. Today maybe only partially, but e.g. event
channels work in a HVM domain even without the Xen platform device.
Grant tables can be made working without the platform device, too,
and I'm already preparing a patch to do exactly that.

The only need for the platform device will then be to have an
interface for unplugging emulated boot devices in favor of the pv
devices. And without the platform device we can just skip that
step without doing any harm, as this can happen only for PVH where
we have no need to do the unplug, or for HVM explicitly configured
to work without platform device needing to continue using the
emulated devices as it is doing today in this case.


Juergen

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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Paolo Bonzini]

On 08/11/2017 13:24, Juergen Gross wrote:
>> My understanding of Xen is very rusty at this point, but I think a
>> "completely" legacy-free HVM domain will still have a PCI bus and the
>> Xen platform device on that bus.
>>
>> A PVH domain just knows how to access the Xen PV features.
>
> A HVM domain does so, too. Today maybe only partially, but e.g. event
> channels work in a HVM domain even without the Xen platform device.
> Grant tables can be made working without the platform device, too,
> and I'm already preparing a patch to do exactly that.

What about assigned PCI devices?  I think they are not PV pcifront for
HVM.  So the main difference in the end is the PCI bus.

Paolo

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Paolo Bonzini]

On 08/11/2017 13:24, Juergen Gross wrote:
>> My understanding of Xen is very rusty at this point, but I think a
>> "completely" legacy-free HVM domain will still have a PCI bus and the
>> Xen platform device on that bus.
>>
>> A PVH domain just knows how to access the Xen PV features.
>
> A HVM domain does so, too. Today maybe only partially, but e.g. event
> channels work in a HVM domain even without the Xen platform device.
> Grant tables can be made working without the platform device, too,
> and I'm already preparing a patch to do exactly that.

What about assigned PCI devices?  I think they are not PV pcifront for
HVM.  So the main difference in the end is the PCI bus.

Paolo

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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Jan Beulich]

>>> On 08.11.17 at 12:55, <jgross@suse.com> wrote:
> On 08/11/17 12:18, Jan Beulich wrote:
>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>> In case we are booted via the default boot entry by a generic loader
>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>> with a device model supporting legacy devices and a PVH guest without
>>> device model.
>>>
>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>> guests.
>>>
>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>> to true if they are met.
>> 
>> This sounds pretty fragile to me: I can't see a reason why a proper
>> HVM guest couldn't come without VGA and RTC. That's not possible
>> today, agreed, but certainly an option down the road if virtualization
>> follows bare metal's road towards being legacy free.
> 
> From guest's perspective: what is the difference between a legacy free
> HVM domain and PVH? In the end the need for differentiating is to avoid
> access to legacy features in PVH as those would require a device model.

My point is that "legacy free" would likely be reached over time (and
even once fully reached, hybrid configurations would be possible).
I.e. there could be a setup with PIC, but with neither VGA nor RTC.
That's still not PVH then. Nor do all legacy features require a device
model in the first place - some of them are being emulated entirely
in the hypervisor.

Furthermore, PVH absolutely requires guest awareness afaict, while
legacy-free pure HVM guests (with an OS only aware of the possible
absence of legacy devices) would still be possible.

Jan

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Jan Beulich]

>>> On 08.11.17 at 12:55, <jgross@suse.com> wrote:
> On 08/11/17 12:18, Jan Beulich wrote:
>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>> In case we are booted via the default boot entry by a generic loader
>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>> with a device model supporting legacy devices and a PVH guest without
>>> device model.
>>>
>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>> guests.
>>>
>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>> to true if they are met.
>> 
>> This sounds pretty fragile to me: I can't see a reason why a proper
>> HVM guest couldn't come without VGA and RTC. That's not possible
>> today, agreed, but certainly an option down the road if virtualization
>> follows bare metal's road towards being legacy free.
> 
> From guest's perspective: what is the difference between a legacy free
> HVM domain and PVH? In the end the need for differentiating is to avoid
> access to legacy features in PVH as those would require a device model.

My point is that "legacy free" would likely be reached over time (and
even once fully reached, hybrid configurations would be possible).
I.e. there could be a setup with PIC, but with neither VGA nor RTC.
That's still not PVH then. Nor do all legacy features require a device
model in the first place - some of them are being emulated entirely
in the hypervisor.

Furthermore, PVH absolutely requires guest awareness afaict, while
legacy-free pure HVM guests (with an OS only aware of the possible
absence of legacy devices) would still be possible.

Jan


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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 13:26, Paolo Bonzini wrote:
> On 08/11/2017 13:24, Juergen Gross wrote:
>>> My understanding of Xen is very rusty at this point, but I think a
>>> "completely" legacy-free HVM domain will still have a PCI bus and the
>>> Xen platform device on that bus.
>>>
>>> A PVH domain just knows how to access the Xen PV features.
>>
>> A HVM domain does so, too. Today maybe only partially, but e.g. event
>> channels work in a HVM domain even without the Xen platform device.
>> Grant tables can be made working without the platform device, too,
>> and I'm already preparing a patch to do exactly that.
> 
> What about assigned PCI devices?  I think they are not PV pcifront for
> HVM.  So the main difference in the end is the PCI bus.

Sure, but this is easily detectable, isn't it?


Juergen

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 13:26, Paolo Bonzini wrote:
> On 08/11/2017 13:24, Juergen Gross wrote:
>>> My understanding of Xen is very rusty at this point, but I think a
>>> "completely" legacy-free HVM domain will still have a PCI bus and the
>>> Xen platform device on that bus.
>>>
>>> A PVH domain just knows how to access the Xen PV features.
>>
>> A HVM domain does so, too. Today maybe only partially, but e.g. event
>> channels work in a HVM domain even without the Xen platform device.
>> Grant tables can be made working without the platform device, too,
>> and I'm already preparing a patch to do exactly that.
> 
> What about assigned PCI devices?  I think they are not PV pcifront for
> HVM.  So the main difference in the end is the PCI bus.

Sure, but this is easily detectable, isn't it?


Juergen


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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 13:31, Jan Beulich wrote:
>>>> On 08.11.17 at 12:55, <jgross@suse.com> wrote:
>> On 08/11/17 12:18, Jan Beulich wrote:
>>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>>> In case we are booted via the default boot entry by a generic loader
>>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>>> with a device model supporting legacy devices and a PVH guest without
>>>> device model.
>>>>
>>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>>> guests.
>>>>
>>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>>> to true if they are met.
>>>
>>> This sounds pretty fragile to me: I can't see a reason why a proper
>>> HVM guest couldn't come without VGA and RTC. That's not possible
>>> today, agreed, but certainly an option down the road if virtualization
>>> follows bare metal's road towards being legacy free.
>>
>> From guest's perspective: what is the difference between a legacy free
>> HVM domain and PVH? In the end the need for differentiating is to avoid
>> access to legacy features in PVH as those would require a device model.
> 
> My point is that "legacy free" would likely be reached over time (and
> even once fully reached, hybrid configurations would be possible).
> I.e. there could be a setup with PIC, but with neither VGA nor RTC.
> That's still not PVH then. Nor do all legacy features require a device
> model in the first place - some of them are being emulated entirely
> in the hypervisor.
> 
> Furthermore, PVH absolutely requires guest awareness afaict, while
> legacy-free pure HVM guests (with an OS only aware of the possible
> absence of legacy devices) would still be possible.

Hmm, where else do you expect PVH awareness to be required? Maybe for
vcpu hotplugging, but this could easily be solved by adding a Xenstore
entry containing the required information. Is there any other problem to
be expected before Xenstore access is possible?


Juergen

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 13:31, Jan Beulich wrote:
>>>> On 08.11.17 at 12:55, <jgross@suse.com> wrote:
>> On 08/11/17 12:18, Jan Beulich wrote:
>>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>>> In case we are booted via the default boot entry by a generic loader
>>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>>> with a device model supporting legacy devices and a PVH guest without
>>>> device model.
>>>>
>>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>>> guests.
>>>>
>>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>>> to true if they are met.
>>>
>>> This sounds pretty fragile to me: I can't see a reason why a proper
>>> HVM guest couldn't come without VGA and RTC. That's not possible
>>> today, agreed, but certainly an option down the road if virtualization
>>> follows bare metal's road towards being legacy free.
>>
>> From guest's perspective: what is the difference between a legacy free
>> HVM domain and PVH? In the end the need for differentiating is to avoid
>> access to legacy features in PVH as those would require a device model.
> 
> My point is that "legacy free" would likely be reached over time (and
> even once fully reached, hybrid configurations would be possible).
> I.e. there could be a setup with PIC, but with neither VGA nor RTC.
> That's still not PVH then. Nor do all legacy features require a device
> model in the first place - some of them are being emulated entirely
> in the hypervisor.
> 
> Furthermore, PVH absolutely requires guest awareness afaict, while
> legacy-free pure HVM guests (with an OS only aware of the possible
> absence of legacy devices) would still be possible.

Hmm, where else do you expect PVH awareness to be required? Maybe for
vcpu hotplugging, but this could easily be solved by adding a Xenstore
entry containing the required information. Is there any other problem to
be expected before Xenstore access is possible?


Juergen

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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Jan Beulich]

>>> On 08.11.17 at 13:45, <jgross@suse.com> wrote:
> On 08/11/17 13:31, Jan Beulich wrote:
>>>>> On 08.11.17 at 12:55, <jgross@suse.com> wrote:
>>> On 08/11/17 12:18, Jan Beulich wrote:
>>>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>>>> In case we are booted via the default boot entry by a generic loader
>>>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>>>> with a device model supporting legacy devices and a PVH guest without
>>>>> device model.
>>>>>
>>>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>>>> guests.
>>>>>
>>>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>>>> to true if they are met.
>>>>
>>>> This sounds pretty fragile to me: I can't see a reason why a proper
>>>> HVM guest couldn't come without VGA and RTC. That's not possible
>>>> today, agreed, but certainly an option down the road if virtualization
>>>> follows bare metal's road towards being legacy free.
>>>
>>> From guest's perspective: what is the difference between a legacy free
>>> HVM domain and PVH? In the end the need for differentiating is to avoid
>>> access to legacy features in PVH as those would require a device model.
>> 
>> My point is that "legacy free" would likely be reached over time (and
>> even once fully reached, hybrid configurations would be possible).
>> I.e. there could be a setup with PIC, but with neither VGA nor RTC.
>> That's still not PVH then. Nor do all legacy features require a device
>> model in the first place - some of them are being emulated entirely
>> in the hypervisor.
>> 
>> Furthermore, PVH absolutely requires guest awareness afaict, while
>> legacy-free pure HVM guests (with an OS only aware of the possible
>> absence of legacy devices) would still be possible.
> 
> Hmm, where else do you expect PVH awareness to be required? Maybe for
> vcpu hotplugging, but this could easily be solved by adding a Xenstore
> entry containing the required information. Is there any other problem to
> be expected before Xenstore access is possible?

Let me ask the question the other way around: What's all the PVH
specific code for under arch/x86/xen/ if there's no difference? One
thing I seem to remember is that getting hold of the ACPI tables
is different between PVH and HVM. Iirc the distinct PVH entry point
is (in part) for that purpose. In the end - with that separate entry
point - it is not really clear to me why any "detection" needs to be
done in the first place: You'd know which mode you're in by knowing
which entry point path you've taken.

Jan

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Jan Beulich]

>>> On 08.11.17 at 13:45, <jgross@suse.com> wrote:
> On 08/11/17 13:31, Jan Beulich wrote:
>>>>> On 08.11.17 at 12:55, <jgross@suse.com> wrote:
>>> On 08/11/17 12:18, Jan Beulich wrote:
>>>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>>>> In case we are booted via the default boot entry by a generic loader
>>>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>>>> with a device model supporting legacy devices and a PVH guest without
>>>>> device model.
>>>>>
>>>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>>>> guests.
>>>>>
>>>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>>>> to true if they are met.
>>>>
>>>> This sounds pretty fragile to me: I can't see a reason why a proper
>>>> HVM guest couldn't come without VGA and RTC. That's not possible
>>>> today, agreed, but certainly an option down the road if virtualization
>>>> follows bare metal's road towards being legacy free.
>>>
>>> From guest's perspective: what is the difference between a legacy free
>>> HVM domain and PVH? In the end the need for differentiating is to avoid
>>> access to legacy features in PVH as those would require a device model.
>> 
>> My point is that "legacy free" would likely be reached over time (and
>> even once fully reached, hybrid configurations would be possible).
>> I.e. there could be a setup with PIC, but with neither VGA nor RTC.
>> That's still not PVH then. Nor do all legacy features require a device
>> model in the first place - some of them are being emulated entirely
>> in the hypervisor.
>> 
>> Furthermore, PVH absolutely requires guest awareness afaict, while
>> legacy-free pure HVM guests (with an OS only aware of the possible
>> absence of legacy devices) would still be possible.
> 
> Hmm, where else do you expect PVH awareness to be required? Maybe for
> vcpu hotplugging, but this could easily be solved by adding a Xenstore
> entry containing the required information. Is there any other problem to
> be expected before Xenstore access is possible?

Let me ask the question the other way around: What's all the PVH
specific code for under arch/x86/xen/ if there's no difference? One
thing I seem to remember is that getting hold of the ACPI tables
is different between PVH and HVM. Iirc the distinct PVH entry point
is (in part) for that purpose. In the end - with that separate entry
point - it is not really clear to me why any "detection" needs to be
done in the first place: You'd know which mode you're in by knowing
which entry point path you've taken.

Jan


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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 13:58, Jan Beulich wrote:
>>>> On 08.11.17 at 13:45, <jgross@suse.com> wrote:
>> On 08/11/17 13:31, Jan Beulich wrote:
>>>>>> On 08.11.17 at 12:55, <jgross@suse.com> wrote:
>>>> On 08/11/17 12:18, Jan Beulich wrote:
>>>>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>>>>> In case we are booted via the default boot entry by a generic loader
>>>>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>>>>> with a device model supporting legacy devices and a PVH guest without
>>>>>> device model.
>>>>>>
>>>>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>>>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>>>>> guests.
>>>>>>
>>>>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>>>>> to true if they are met.
>>>>>
>>>>> This sounds pretty fragile to me: I can't see a reason why a proper
>>>>> HVM guest couldn't come without VGA and RTC. That's not possible
>>>>> today, agreed, but certainly an option down the road if virtualization
>>>>> follows bare metal's road towards being legacy free.
>>>>
>>>> From guest's perspective: what is the difference between a legacy free
>>>> HVM domain and PVH? In the end the need for differentiating is to avoid
>>>> access to legacy features in PVH as those would require a device model.
>>>
>>> My point is that "legacy free" would likely be reached over time (and
>>> even once fully reached, hybrid configurations would be possible).
>>> I.e. there could be a setup with PIC, but with neither VGA nor RTC.
>>> That's still not PVH then. Nor do all legacy features require a device
>>> model in the first place - some of them are being emulated entirely
>>> in the hypervisor.
>>>
>>> Furthermore, PVH absolutely requires guest awareness afaict, while
>>> legacy-free pure HVM guests (with an OS only aware of the possible
>>> absence of legacy devices) would still be possible.
>>
>> Hmm, where else do you expect PVH awareness to be required? Maybe for
>> vcpu hotplugging, but this could easily be solved by adding a Xenstore
>> entry containing the required information. Is there any other problem to
>> be expected before Xenstore access is possible?
> 
> Let me ask the question the other way around: What's all the PVH
> specific code for under arch/x86/xen/ if there's no difference? One

Most of it is for early boot when coming through the PVH specific
boot entry.

> thing I seem to remember is that getting hold of the ACPI tables
> is different between PVH and HVM. Iirc the distinct PVH entry point
> is (in part) for that purpose. In the end - with that separate entry
> point - it is not really clear to me why any "detection" needs to be
> done in the first place: You'd know which mode you're in by knowing
> which entry point path you've taken.

Its all in the commit message: I am trying to enable a boot loader to
use the default kernel boot entry for PVH. This will reduce the needed
modifications in the loader.

Regarding ACPI tables: current PVH implementation in Linux kernel
seems not to make use of the special information presented in the boot
information block.


Juergen

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 13:58, Jan Beulich wrote:
>>>> On 08.11.17 at 13:45, <jgross@suse.com> wrote:
>> On 08/11/17 13:31, Jan Beulich wrote:
>>>>>> On 08.11.17 at 12:55, <jgross@suse.com> wrote:
>>>> On 08/11/17 12:18, Jan Beulich wrote:
>>>>>>>> On 08.11.17 at 10:07, <jgross@suse.com> wrote:
>>>>>> In case we are booted via the default boot entry by a generic loader
>>>>>> like grub or OVMF it is necessary to distinguish between a HVM guest
>>>>>> with a device model supporting legacy devices and a PVH guest without
>>>>>> device model.
>>>>>>
>>>>>> PVH guests will always have x86_platform.legacy.no_vga set and
>>>>>> x86_platform.legacy.rtc cleared, while both won't be true for HVM
>>>>>> guests.
>>>>>>
>>>>>> Test for both conditions in the guest_late_init hook and set xen_pvh
>>>>>> to true if they are met.
>>>>>
>>>>> This sounds pretty fragile to me: I can't see a reason why a proper
>>>>> HVM guest couldn't come without VGA and RTC. That's not possible
>>>>> today, agreed, but certainly an option down the road if virtualization
>>>>> follows bare metal's road towards being legacy free.
>>>>
>>>> From guest's perspective: what is the difference between a legacy free
>>>> HVM domain and PVH? In the end the need for differentiating is to avoid
>>>> access to legacy features in PVH as those would require a device model.
>>>
>>> My point is that "legacy free" would likely be reached over time (and
>>> even once fully reached, hybrid configurations would be possible).
>>> I.e. there could be a setup with PIC, but with neither VGA nor RTC.
>>> That's still not PVH then. Nor do all legacy features require a device
>>> model in the first place - some of them are being emulated entirely
>>> in the hypervisor.
>>>
>>> Furthermore, PVH absolutely requires guest awareness afaict, while
>>> legacy-free pure HVM guests (with an OS only aware of the possible
>>> absence of legacy devices) would still be possible.
>>
>> Hmm, where else do you expect PVH awareness to be required? Maybe for
>> vcpu hotplugging, but this could easily be solved by adding a Xenstore
>> entry containing the required information. Is there any other problem to
>> be expected before Xenstore access is possible?
> 
> Let me ask the question the other way around: What's all the PVH
> specific code for under arch/x86/xen/ if there's no difference? One

Most of it is for early boot when coming through the PVH specific
boot entry.

> thing I seem to remember is that getting hold of the ACPI tables
> is different between PVH and HVM. Iirc the distinct PVH entry point
> is (in part) for that purpose. In the end - with that separate entry
> point - it is not really clear to me why any "detection" needs to be
> done in the first place: You'd know which mode you're in by knowing
> which entry point path you've taken.

Its all in the commit message: I am trying to enable a boot loader to
use the default kernel boot entry for PVH. This will reduce the needed
modifications in the loader.

Regarding ACPI tables: current PVH implementation in Linux kernel
seems not to make use of the special information presented in the boot
information block.


Juergen

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

Re: [PATCH 0/3] x86/xen: support booting PVH guest via standard boot path

[Boris Ostrovsky]

On 11/08/2017 04:07 AM, Juergen Gross wrote:
> Booting a Xen PVH guest requires a special boot entry as it is
> mandatory to setup some Xen-specific interfaces rather early. When grub
> or OVMF are used as boot loaders, however, those will fill the boot
> parameters in zeropage and there is no longer a need to do something
> PVH specific in the early boot path.
>
> This patch series adds support for that scenario by identifying PVH
> environment and doing the required init steps via Xen hooks instead of
> using a dedicated boot entry.
>
> The dedicated entry is still needed for support of Dom0 running in PVH
> mode as in this case there is no grub or OVMF involved for filling in
> the boot parameters.

We are going to continue supporting direct boot of unprivileged guests
too so this entry point will be needed not for dom0 only.

-boris

>
> Juergen Gross (3):
>   x86/acpi: add test for ACPI_FADT_NO_VGA
>   x86: add guest_late_init hook to hypervisor_x86 structure
>   x86/xen: use guest_late_init to detect Xen PVH guest
>
>  arch/x86/include/asm/hypervisor.h | 11 +++++++++++
>  arch/x86/include/asm/kvm_para.h   |  2 --
>  arch/x86/include/asm/x86_init.h   |  1 +
>  arch/x86/kernel/acpi/boot.c       |  5 +++++
>  arch/x86/kernel/kvm.c             |  3 ++-
>  arch/x86/kernel/setup.c           |  2 +-
>  arch/x86/xen/enlighten_hvm.c      | 24 ++++++++++++++++++++++--
>  arch/x86/xen/enlighten_pvh.c      |  9 ---------
>  8 files changed, 42 insertions(+), 15 deletions(-)
>

Re: [PATCH 0/3] x86/xen: support booting PVH guest via standard boot path

[Boris Ostrovsky]

On 11/08/2017 04:07 AM, Juergen Gross wrote:
> Booting a Xen PVH guest requires a special boot entry as it is
> mandatory to setup some Xen-specific interfaces rather early. When grub
> or OVMF are used as boot loaders, however, those will fill the boot
> parameters in zeropage and there is no longer a need to do something
> PVH specific in the early boot path.
>
> This patch series adds support for that scenario by identifying PVH
> environment and doing the required init steps via Xen hooks instead of
> using a dedicated boot entry.
>
> The dedicated entry is still needed for support of Dom0 running in PVH
> mode as in this case there is no grub or OVMF involved for filling in
> the boot parameters.

We are going to continue supporting direct boot of unprivileged guests
too so this entry point will be needed not for dom0 only.

-boris

>
> Juergen Gross (3):
>   x86/acpi: add test for ACPI_FADT_NO_VGA
>   x86: add guest_late_init hook to hypervisor_x86 structure
>   x86/xen: use guest_late_init to detect Xen PVH guest
>
>  arch/x86/include/asm/hypervisor.h | 11 +++++++++++
>  arch/x86/include/asm/kvm_para.h   |  2 --
>  arch/x86/include/asm/x86_init.h   |  1 +
>  arch/x86/kernel/acpi/boot.c       |  5 +++++
>  arch/x86/kernel/kvm.c             |  3 ++-
>  arch/x86/kernel/setup.c           |  2 +-
>  arch/x86/xen/enlighten_hvm.c      | 24 ++++++++++++++++++++++--
>  arch/x86/xen/enlighten_pvh.c      |  9 ---------
>  8 files changed, 42 insertions(+), 15 deletions(-)
>


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

Re: [PATCH 0/3] x86/xen: support booting PVH guest via standard boot path

[Juergen Gross]

On 08/11/17 14:37, Boris Ostrovsky wrote:
> On 11/08/2017 04:07 AM, Juergen Gross wrote:
>> Booting a Xen PVH guest requires a special boot entry as it is
>> mandatory to setup some Xen-specific interfaces rather early. When grub
>> or OVMF are used as boot loaders, however, those will fill the boot
>> parameters in zeropage and there is no longer a need to do something
>> PVH specific in the early boot path.
>>
>> This patch series adds support for that scenario by identifying PVH
>> environment and doing the required init steps via Xen hooks instead of
>> using a dedicated boot entry.
>>
>> The dedicated entry is still needed for support of Dom0 running in PVH
>> mode as in this case there is no grub or OVMF involved for filling in
>> the boot parameters.
> 
> We are going to continue supporting direct boot of unprivileged guests
> too so this entry point will be needed not for dom0 only.

Sure, but using e.g. grub in this case would be an alternative. For Dom0
this alternative isn't existing. So this entry is mandatory, not a "nice
to have".


Juergen

Re: [PATCH 0/3] x86/xen: support booting PVH guest via standard boot path

[Juergen Gross]

On 08/11/17 14:37, Boris Ostrovsky wrote:
> On 11/08/2017 04:07 AM, Juergen Gross wrote:
>> Booting a Xen PVH guest requires a special boot entry as it is
>> mandatory to setup some Xen-specific interfaces rather early. When grub
>> or OVMF are used as boot loaders, however, those will fill the boot
>> parameters in zeropage and there is no longer a need to do something
>> PVH specific in the early boot path.
>>
>> This patch series adds support for that scenario by identifying PVH
>> environment and doing the required init steps via Xen hooks instead of
>> using a dedicated boot entry.
>>
>> The dedicated entry is still needed for support of Dom0 running in PVH
>> mode as in this case there is no grub or OVMF involved for filling in
>> the boot parameters.
> 
> We are going to continue supporting direct boot of unprivileged guests
> too so this entry point will be needed not for dom0 only.

Sure, but using e.g. grub in this case would be an alternative. For Dom0
this alternative isn't existing. So this entry is mandatory, not a "nice
to have".


Juergen


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

Re: [PATCH 0/3] x86/xen: support booting PVH guest via standard boot path

[Boris Ostrovsky]

On 11/08/2017 08:40 AM, Juergen Gross wrote:
> On 08/11/17 14:37, Boris Ostrovsky wrote:
>> On 11/08/2017 04:07 AM, Juergen Gross wrote:
>>> Booting a Xen PVH guest requires a special boot entry as it is
>>> mandatory to setup some Xen-specific interfaces rather early. When grub
>>> or OVMF are used as boot loaders, however, those will fill the boot
>>> parameters in zeropage and there is no longer a need to do something
>>> PVH specific in the early boot path.
>>>
>>> This patch series adds support for that scenario by identifying PVH
>>> environment and doing the required init steps via Xen hooks instead of
>>> using a dedicated boot entry.
>>>
>>> The dedicated entry is still needed for support of Dom0 running in PVH
>>> mode as in this case there is no grub or OVMF involved for filling in
>>> the boot parameters.
>> We are going to continue supporting direct boot of unprivileged guests
>> too so this entry point will be needed not for dom0 only.
> Sure, but using e.g. grub in this case would be an alternative. For Dom0
> this alternative isn't existing. So this entry is mandatory, not a "nice
> to have".

Right, I was just pointing out that the way the message is phrased makes
it sounds (to me at least) as if dom0 is the only reason for the
dedicated entry point.

-boris

Re: [PATCH 0/3] x86/xen: support booting PVH guest via standard boot path

[Boris Ostrovsky]

On 11/08/2017 08:40 AM, Juergen Gross wrote:
> On 08/11/17 14:37, Boris Ostrovsky wrote:
>> On 11/08/2017 04:07 AM, Juergen Gross wrote:
>>> Booting a Xen PVH guest requires a special boot entry as it is
>>> mandatory to setup some Xen-specific interfaces rather early. When grub
>>> or OVMF are used as boot loaders, however, those will fill the boot
>>> parameters in zeropage and there is no longer a need to do something
>>> PVH specific in the early boot path.
>>>
>>> This patch series adds support for that scenario by identifying PVH
>>> environment and doing the required init steps via Xen hooks instead of
>>> using a dedicated boot entry.
>>>
>>> The dedicated entry is still needed for support of Dom0 running in PVH
>>> mode as in this case there is no grub or OVMF involved for filling in
>>> the boot parameters.
>> We are going to continue supporting direct boot of unprivileged guests
>> too so this entry point will be needed not for dom0 only.
> Sure, but using e.g. grub in this case would be an alternative. For Dom0
> this alternative isn't existing. So this entry is mandatory, not a "nice
> to have".

Right, I was just pointing out that the way the message is phrased makes
it sounds (to me at least) as if dom0 is the only reason for the
dedicated entry point.

-boris

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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Boris Ostrovsky]

On 11/08/2017 08:36 AM, Juergen Gross wrote:
>
> Regarding ACPI tables: current PVH implementation in Linux kernel
> seems not to make use of the special information presented in the boot
> information block.

It will need to do so for dom0 (and, then, for simplicity, for all PVH
guests).

-boris

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Boris Ostrovsky]

On 11/08/2017 08:36 AM, Juergen Gross wrote:
>
> Regarding ACPI tables: current PVH implementation in Linux kernel
> seems not to make use of the special information presented in the boot
> information block.

It will need to do so for dom0 (and, then, for simplicity, for all PVH
guests).

-boris


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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 15:10, Boris Ostrovsky wrote:
> On 11/08/2017 08:36 AM, Juergen Gross wrote:
>>
>> Regarding ACPI tables: current PVH implementation in Linux kernel
>> seems not to make use of the special information presented in the boot
>> information block.
> 
> It will need to do so for dom0 (and, then, for simplicity, for all PVH
> guests).

What about: for all PVH guests booted via the PVH specific boot entry?
A guest booted via the default boot entry won't know it is PVH until
ACPI tables have been scanned.


Juergen

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Juergen Gross]

On 08/11/17 15:10, Boris Ostrovsky wrote:
> On 11/08/2017 08:36 AM, Juergen Gross wrote:
>>
>> Regarding ACPI tables: current PVH implementation in Linux kernel
>> seems not to make use of the special information presented in the boot
>> information block.
> 
> It will need to do so for dom0 (and, then, for simplicity, for all PVH
> guests).

What about: for all PVH guests booted via the PVH specific boot entry?
A guest booted via the default boot entry won't know it is PVH until
ACPI tables have been scanned.


Juergen


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

Re: [Xen-devel] [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Boris Ostrovsky]

On 11/08/2017 09:17 AM, Juergen Gross wrote:
> On 08/11/17 15:10, Boris Ostrovsky wrote:
>> On 11/08/2017 08:36 AM, Juergen Gross wrote:
>>> Regarding ACPI tables: current PVH implementation in Linux kernel
>>> seems not to make use of the special information presented in the boot
>>> information block.
>> It will need to do so for dom0 (and, then, for simplicity, for all PVH
>> guests).
> What about: for all PVH guests booted via the PVH specific boot entry?
> A guest booted via the default boot entry won't know it is PVH until
> ACPI tables have been scanned.

Right. Guest booted from default entry will have to discover RSDP in the
usual way.

-boris

Re: [PATCH 3/3] x86/xen: use guest_late_init to detect Xen PVH guest

[Boris Ostrovsky]

On 11/08/2017 09:17 AM, Juergen Gross wrote:
> On 08/11/17 15:10, Boris Ostrovsky wrote:
>> On 11/08/2017 08:36 AM, Juergen Gross wrote:
>>> Regarding ACPI tables: current PVH implementation in Linux kernel
>>> seems not to make use of the special information presented in the boot
>>> information block.
>> It will need to do so for dom0 (and, then, for simplicity, for all PVH
>> guests).
> What about: for all PVH guests booted via the PVH specific boot entry?
> A guest booted via the default boot entry won't know it is PVH until
> ACPI tables have been scanned.

Right. Guest booted from default entry will have to discover RSDP in the
usual way.

-boris

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