Re: [Xen-devel] [PATCH 3/7] xen/hvm: Xen PV extension of HVM initialization

From: Jeremy Fitzhardinge
Date: Mon Mar 01 2010 - 20:03:02 EST

On 03/01/2010 01:38 AM, Sheng Yang wrote:
The PV extension of HVM(once known as Hybrid) is started from real mode like
HVM guest, but also with a component based PV feature selection(e.g. PV halt,
PV timer, event channel, then PV drivers). So guest can takes the advantages
of both H/W virtualization and Para-Virtualization.

This patch introduced the PV extension of HVM guest initialization.

Guest would detect the capability using CPUID 0x40000002.edx, then call
HVMOP_enable_pv hypercall to enable pv support in hypervisor.

Signed-off-by: Sheng Yang<sheng@xxxxxxxxxxxxxxx>
Signed-off-by: Yaozu (Eddie) Dong<eddie.dong@xxxxxxxxx>
---
arch/x86/include/asm/xen/cpuid.h | 5 ++
arch/x86/xen/enlighten.c | 115 ++++++++++++++++++++++++++++++++++++
arch/x86/xen/irq.c | 21 +++++++
arch/x86/xen/xen-head.S | 6 ++
arch/x86/xen/xen-ops.h | 1 +
include/xen/interface/hvm/hvm_op.h | 7 ++
include/xen/xen.h | 9 +++
7 files changed, 164 insertions(+), 0 deletions(-)

diff --git a/arch/x86/include/asm/xen/cpuid.h b/arch/x86/include/asm/xen/cpuid.h
index 8787f03..a93c851 100644
--- a/arch/x86/include/asm/xen/cpuid.h
+++ b/arch/x86/include/asm/xen/cpuid.h
@@ -65,4 +65,9 @@
#define _XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD 0
#define XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD (1u<<0)

+#define _XEN_CPUID_FEAT2_HVM_PV 0
+#define XEN_CPUID_FEAT2_HVM_PV (1u<<0)
+#define _XEN_CPUID_FEAT2_HVM_PV_EVTCHN 1
+#define XEN_CPUID_FEAT2_HVM_PV_EVTCHN (1u<<1)
+
#endif /* __XEN_PUBLIC_ARCH_X86_CPUID_H__ */
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 36daccb..fdb9664 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -34,6 +34,8 @@
#include<xen/interface/version.h>
#include<xen/interface/physdev.h>
#include<xen/interface/vcpu.h>
+#include<xen/interface/memory.h>
+#include<xen/interface/hvm/hvm_op.h>
#include<xen/features.h>
#include<xen/page.h>
#include<xen/hvc-console.h>
@@ -43,6 +45,7 @@
#include<asm/page.h>
#include<asm/xen/hypercall.h>
#include<asm/xen/hypervisor.h>
+#include<asm/xen/cpuid.h>
#include<asm/fixmap.h>
#include<asm/processor.h>
#include<asm/proto.h>
@@ -1198,3 +1201,115 @@ asmlinkage void __init xen_start_kernel(void)
x86_64_start_reservations((char *)__pa_symbol(&boot_params));
#endif
}
+
+static void __init xen_hvm_pv_banner(void)
+{
+ unsigned version = HYPERVISOR_xen_version(XENVER_version, NULL);
+ struct xen_extraversion extra;
+ HYPERVISOR_xen_version(XENVER_extraversion,&extra);
+
+ printk(KERN_INFO "Booting PV featured HVM kernel on %s\n",
+ pv_info.name);
+ printk(KERN_INFO "Xen version: %d.%d%s\n",
+ version>> 16, version& 0xffff, extra.extraversion);
+}
+
+static int xen_para_available(void)
+{
+ uint32_t eax, ebx, ecx, edx;
+ cpuid(XEN_CPUID_LEAF(0),&eax,&ebx,&ecx,&edx);
+
+ if (ebx == XEN_CPUID_SIGNATURE_EBX&&
+ ecx == XEN_CPUID_SIGNATURE_ECX&&
+ edx == XEN_CPUID_SIGNATURE_EDX&&
+ ((eax - XEN_CPUID_LEAF(0))>= 2))
+ return 1;
+
+ return 0;
+}
+
+u32 xen_hvm_pv_status;
+EXPORT_SYMBOL_GPL(xen_hvm_pv_status);
+
+static int enable_hvm_pv(u64 flags)
+{
+ struct xen_hvm_pv_type a;
+
+ a.domid = DOMID_SELF;
+ a.flags = flags;
+ return HYPERVISOR_hvm_op(HVMOP_enable_pv,&a);
+}
+
+static int init_hvm_pv_info(void)
+{
+ uint32_t ecx, edx, pages, msr;
+ u64 pfn;
+
+ if (!xen_para_available())
+ return -EINVAL;
+
+ cpuid(XEN_CPUID_LEAF(2),&pages,&msr,&ecx,&edx);
+
+ /* Check if hvm_pv mode is supported */
+ if (!(edx& XEN_CPUID_FEAT2_HVM_PV))
+ return -ENODEV;
+
+ xen_hvm_pv_status = XEN_HVM_PV_ENABLED;

Why use this? Why not just set the domain type to HVM, and leave the "status" field as a bitset of available paravirtualizations?

+
+ /* We only support 1 page of hypercall for now */
+ if (pages != 1)
+ return -ENOMEM;
+
+ pfn = __pa(hypercall_page);
+ wrmsrl(msr, pfn);
+
+ xen_setup_features();
+
+ x86_init.oem.banner = xen_hvm_pv_banner;
+ pv_info = xen_info;
+ pv_info.kernel_rpl = 0;
+
+ return 0;
+}
+
+extern struct shared_info shared_info_page;
+
+static void __init init_shared_info(void)
+{
+ struct xen_add_to_physmap xatp;
+
+ xatp.domid = DOMID_SELF;
+ xatp.idx = 0;
+ xatp.space = XENMAPSPACE_shared_info;
+ xatp.gpfn = __pa(&shared_info_page)>> PAGE_SHIFT;
+ if (HYPERVISOR_memory_op(XENMEM_add_to_physmap,&xatp))
+ BUG();
+
+ HYPERVISOR_shared_info = (struct shared_info *)&shared_info_page;
+
+ /* Don't do the full vcpu_info placement stuff until we have a
+ possible map and a non-dummy shared_info. */

Is this comment meaningful here? This is the real shared info at this point, no? Are you going to support vcpu_info placement?

+ per_cpu(xen_vcpu, 0) =&HYPERVISOR_shared_info->vcpu_info[0];
+}
+
+void __init xen_guest_init(void)
+{
+#ifdef CONFIG_X86_32
+ return;
+#else
+ int r;
+
+ /* Ensure the we won't confused with PV */
+ if (xen_domain_type == XEN_PV_DOMAIN)
+ return;

Aren't you specifically testing for xen_domain_type == NATIVE here? If its anything else, then it means we're either PV, or have become an HVM domain some other way (like probing for the Xen platform PCI device).



+
+ r = init_hvm_pv_info();
+ if (r< 0)
+ return;
+
+ init_shared_info();
+
+ xen_hvm_pv_init_irq_ops();
+#endif
+}

Can you split all this off into a new file. It doesn't seem to have any dependencies on the rest of enlighten.c, and I've been trying to disaggregate it anyway.

+
diff --git a/arch/x86/xen/irq.c b/arch/x86/xen/irq.c
index 9d30105..fadaa97 100644
--- a/arch/x86/xen/irq.c
+++ b/arch/x86/xen/irq.c
@@ -131,3 +131,24 @@ void __init xen_init_irq_ops()
pv_irq_ops = xen_irq_ops;
x86_init.irqs.intr_init = xen_init_IRQ;
}
+
+static void xen_hvm_pv_safe_halt(void)
+{
+ /* Do local_irq_enable() explicitly in hvm_pv guest */
+ local_irq_enable();
+ xen_safe_halt();
+}
+
+static void xen_hvm_pv_halt(void)
+{
+ if (irqs_disabled())
+ HYPERVISOR_vcpu_op(VCPUOP_down, smp_processor_id(), NULL);
+ else
+ xen_hvm_pv_safe_halt();
+}
+
+void __init xen_hvm_pv_init_irq_ops(void)
+{
+ pv_irq_ops.safe_halt = xen_hvm_pv_safe_halt;
+ pv_irq_ops.halt = xen_hvm_pv_halt;
+}

It would be better to make this patch purely common infrastructure, and make specific features (like hvm+pv halt) separate patches (one per patch).

diff --git a/arch/x86/xen/xen-head.S b/arch/x86/xen/xen-head.S
index 1a5ff24..26041ce 100644
--- a/arch/x86/xen/xen-head.S
+++ b/arch/x86/xen/xen-head.S
@@ -33,6 +33,12 @@ ENTRY(hypercall_page)
.skip PAGE_SIZE_asm
.popsection

+.pushsection .data
+ .align PAGE_SIZE_asm
+ENTRY(shared_info_page)
+ .skip PAGE_SIZE_asm
+.popsection

Why does this need to be defined in asm? Can't it be either allocated or defined in C?

+
ELFNOTE(Xen, XEN_ELFNOTE_GUEST_OS, .asciz "linux")
ELFNOTE(Xen, XEN_ELFNOTE_GUEST_VERSION, .asciz "2.6")
ELFNOTE(Xen, XEN_ELFNOTE_XEN_VERSION, .asciz "xen-3.0")
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index f9153a3..cc00760 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -41,6 +41,7 @@ void xen_vcpu_restore(void);
void __init xen_build_dynamic_phys_to_machine(void);

void xen_init_irq_ops(void);
+void xen_hvm_pv_init_irq_ops(void);
void xen_setup_timer(int cpu);
void xen_setup_runstate_info(int cpu);
void xen_teardown_timer(int cpu);
diff --git a/include/xen/interface/hvm/hvm_op.h b/include/xen/interface/hvm/hvm_op.h
index 7c74ba4..0ce8a26 100644
--- a/include/xen/interface/hvm/hvm_op.h
+++ b/include/xen/interface/hvm/hvm_op.h
@@ -69,4 +69,11 @@ DEFINE_GUEST_HANDLE_STRUCT(xen_hvm_set_pci_link_route);
/* Flushes all VCPU TLBs: @arg must be NULL. */
#define HVMOP_flush_tlbs 5

+#define HVMOP_enable_pv 9
+struct xen_hvm_pv_type {
+ domid_t domid;
+ uint32_t flags;
+#define HVM_PV_EVTCHN (1ull<<1)
+};
+
#endif /* __XEN_PUBLIC_HVM_HVM_OP_H__ */
diff --git a/include/xen/xen.h b/include/xen/xen.h
index a164024..9bb92e5 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -9,6 +9,7 @@ enum xen_domain_type {

#ifdef CONFIG_XEN
extern enum xen_domain_type xen_domain_type;
+extern void xen_guest_init(void);
#else
#define xen_domain_type XEN_NATIVE
#endif
@@ -19,6 +20,14 @@ extern enum xen_domain_type xen_domain_type;
#define xen_hvm_domain() (xen_domain()&& \
xen_domain_type == XEN_HVM_DOMAIN)

+#define XEN_HVM_PV_ENABLED (1u<< 0)

Why have this? We already have xen_domain_type which will either be XEN_NATIVE (ie, either real native, or on some fully emulated environment we have no specific optimisations for), or XEN_HVM_DOMAIN (we know we're running under Xen as an HVM domain).

+#define XEN_HVM_PV_EVTCHN_ENABLED (1u<< 1)
+extern u32 xen_hvm_pv_status;

I think "status" is a misnomer here. Isn't it specifically a set of PV features which are active?

+
+#define xen_hvm_pv_enabled() (xen_hvm_pv_status& XEN_HVM_PV_ENABLED)
+#define xen_hvm_pv_evtchn_enabled() (xen_hvm_pv_enabled()&& \
+ (xen_hvm_pv_status& XEN_HVM_PV_EVTCHN_ENABLED))

Testing for xen_hvm_pv_enabled() should be redundant; surely the status/feature flag won't be set unless the environment supports the feature, and if it does it doesn't really matter what the domain type is.

+
#ifdef CONFIG_XEN_DOM0
#include<xen/interface/xen.h>
#include<asm/xen/hypervisor.h>

J
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/