[PATCH v3 0/4] Expose HW APIC virtualization support to HVM guests

[PATCH v3 0/4] Expose HW APIC virtualization support to HVM guests

[Boris Ostrovsky]

Version 3:
* Removed sysctl for querying hypervisor leaf from libxc, replaced it with
  prefixed CPUID instruction
* Limited ability change hypervisor leaves to 0x40000000
* Fixed IS_HYPERVISOR_LEAF macro

Version 2:
* Added ability to specify hypervisor CPUID leaves in config file (this requires
  new sysctl)
* Use 2 bits to indicate what is supported --- one for APIC memory access and the
  other for x2APIC. Still not sure whether virtual interrupt delivery should be
  exposed as well.



HVM guests running on HW that supports HW APIC virtualization features
(APIC-register virtualization, virtual interrupt delivery, etc) may
want to use APIC instead of hvm_pirqs. Since we are not guaranteed to
have these features on VMX (for example, there is a boot option to
turn it off) and there is no such support on SVM we need to make the
guest aware that its APIC accesses may not be so bad.

CPUID seems to be a good way to provide this info to the guest.

Having a guest switch to APIC shows fairly good impact on number of
VMEXITs. For example, with a pass-through NIC, netperf sees almost
half as many. Here are results for 'xentrace -e 0x00083fff -c 2 -D -T 2'

(The guest here essentially turned off XENFEAT_hvm_pirqs but we may
want to use APIC for MSI interrupts only and leave pirqs for gsi).


[root@ovs105 virt]#  cat orig  |xentrace_format  ~/xen/tools/xentrace/formats | awk  '{print $5}' | sort  | uniq -c
     94 cpu_change
  13944 HLT
  26341 INJ_VIRQ
  12054 INTR
  30784 INTR_WINDOW
  10126 TRAP
 124783 VMENTRY
 124782 VMEXIT
  59217 VMMCALL
     35 wrap_buffer
 
 
[root@ovs105 virt]#  cat apicv  |xentrace_format  ~/xen/tools/xentrace/formats | awk  '{print $5}' | sort  | uniq -c
     49 cpu_change
  16157 HLT
     31 INJ_VIRQ
  10652 INTR
     38 INTR_WINDOW
     10 NPF
  10286 TRAP
  71269 VMENTRY
  71269 VMEXIT
  34129 VMMCALL
     15 wrap_buffer

The difference is even larger when the guest is busy.

These results are in line with what has been reported for KVM. For example
http://events.linuxfoundation.org/sites/events/files/cojp13_natapov.pdf
http://www.linuxplumbersconf.org/2012/wp-content/uploads/2012/09/2012-lpc-virt-intel-vt-feat-nakajima.pdf

I am also not sure whether (cpu_has_vmx_apic_reg_virt &
cpu_has_vmx_virtualize_x2apic_mode) is sufficient to declare full HW
APIC support to a guest. The tests show ~95K VMEXITs when virtual
interrupt delivery and posted interrupts are turned off so there
appears to still be some benefit. I suppose we can use another CPUID
bit for these two (although I am not particularly eager to do this).

Boris Ostrovsky (4):
  xen/libxc: Allow changes to hypervisor CPUID leaf from config file
  x86/hvm: Revert 80ecb40362365ba77e68fc609de8bd3b7208ae19
  x86/hvm: Add HVM-specific hypervisor CPUID leaf
  x86/hvm: Indicate avaliability of HW support of APIC virtualization
    to HVM guests

 tools/libxc/xc_cpuid_x86.c          |   31 ++++++++++++++++++++++++++-----
 xen/arch/x86/domain.c               |   19 ++++++++++++++++---
 xen/arch/x86/hvm/hvm.c              |   16 ++++++++++++++++
 xen/arch/x86/hvm/vmx/vmx.c          |   12 ++++++++++++
 xen/arch/x86/traps.c                |   18 +++++++++---------
 xen/include/asm-x86/domain.h        |    7 +++++++
 xen/include/asm-x86/hvm/hvm.h       |    7 +++++++
 xen/include/public/arch-x86/cpuid.h |   10 ++++++++++
 8 files changed, 103 insertions(+), 17 deletions(-)

-- 
1.7.10.4

[PATCH v3 1/4] xen/libxc: Allow changes to hypervisor CPUID leaf from config file

[Boris Ostrovsky]

Currently only "real" cpuid leaves can be overwritten by users via
'cpuid' option in the configuration file. This patch provides ability to
do the same for hypervisor leaves (but for now only 0x40000000 is allowed).

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 tools/libxc/xc_cpuid_x86.c   |   31 ++++++++++++++++++++++++++-----
 xen/arch/x86/domain.c        |   19 ++++++++++++++++---
 xen/arch/x86/traps.c         |    3 +++
 xen/include/asm-x86/domain.h |    7 +++++++
 4 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index bbbf9b8..4920c7e 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -33,6 +33,8 @@
 #define DEF_MAX_INTELEXT  0x80000008u
 #define DEF_MAX_AMDEXT    0x8000001cu
 
+#define IS_HYPERVISOR_LEAF(idx) (((idx) & 0xffff0000) == 0x40000000)
+
 static int hypervisor_is_64bit(xc_interface *xch)
 {
     xen_capabilities_info_t xen_caps = "";
@@ -43,22 +45,29 @@ static int hypervisor_is_64bit(xc_interface *xch)
 static void cpuid(const unsigned int *input, unsigned int *regs)
 {
     unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1];
+    uint8_t is_hyp = IS_HYPERVISOR_LEAF(input[0]);
 #ifdef __i386__
     /* Use the stack to avoid reg constraint failures with some gcc flags */
     asm (
         "push %%ebx; push %%edx\n\t"
-        "cpuid\n\t"
+        "testb $0xff,%5\n\t"
+        "jz 1f\n\t"
+        XEN_EMULATE_PREFIX
+        "1: cpuid\n\t"
         "mov %%ebx,4(%4)\n\t"
         "mov %%edx,12(%4)\n\t"
         "pop %%edx; pop %%ebx\n\t"
         : "=a" (regs[0]), "=c" (regs[2])
-        : "0" (input[0]), "1" (count), "S" (regs)
+        : "0" (input[0]), "1" (count), "S" (regs), "m" (is_hyp)
         : "memory" );
 #else
     asm (
-        "cpuid"
+        "testb $0xff,%6\n\t"
+        "jz 1f\n\t"
+        XEN_EMULATE_PREFIX
+        "1: cpuid"
         : "=a" (regs[0]), "=b" (regs[1]), "=c" (regs[2]), "=d" (regs[3])
-        : "0" (input[0]), "2" (count) );
+        : "0" (input[0]), "2" (count), "m" (is_hyp) );
 #endif
 }
 
@@ -555,6 +564,15 @@ static int xc_cpuid_policy(
 {
     xc_dominfo_t        info;
 
+    if ( IS_HYPERVISOR_LEAF(input[0]) )
+    {
+        /* Only leaf 1 can be modified */
+        if ( input[0] == 0x40000000 )
+            return 0;
+        else
+            return -EACCES;
+    }
+
     if ( xc_domain_getinfo(xch, domid, 1, &info) == 0 )
         return -EINVAL;
 
@@ -757,7 +775,10 @@ int xc_cpuid_set(
     cpuid(input, regs);
 
     memcpy(polregs, regs, sizeof(regs));
-    xc_cpuid_policy(xch, domid, input, polregs);
+    rc = xc_cpuid_policy(xch, domid, input, polregs);
+    if ( rc )
+        return rc;
+
 
     for ( i = 0; i < 4; i++ )
     {
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index c42a079..98e2b5f 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1997,7 +1997,7 @@ void arch_dump_vcpu_info(struct vcpu *v)
         vpmu_dump(v);
 }
 
-void domain_cpuid(
+bool_t domain_cpuid_exists(
     struct domain *d,
     unsigned int  input,
     unsigned int  sub_input,
@@ -2030,11 +2030,24 @@ void domain_cpuid(
                  !d->disable_migrate && !d->arch.vtsc )
                 *edx &= ~(1u<<8); /* TSC Invariant */
 
-            return;
+            return 1;
         }
     }
 
-    *eax = *ebx = *ecx = *edx = 0;
+    return 0;
+}
+
+void domain_cpuid(
+    struct domain *d,
+    unsigned int  input,
+    unsigned int  sub_input,
+    unsigned int  *eax,
+    unsigned int  *ebx,
+    unsigned int  *ecx,
+    unsigned int  *edx)
+{
+    if ( !domain_cpuid_exists(d, input, sub_input, eax, ebx, ecx, edx) )
+        *eax = *ebx = *ecx = *edx = 0;
 }
 
 void vcpu_kick(struct vcpu *v)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index c462317..e1f39d9 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -690,6 +690,9 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
     if ( idx > limit ) 
         return 0;
 
+    if ( domain_cpuid_exists(d, base + idx, sub_idx, eax, ebx, ecx, edx) )
+        return 1;
+
     switch ( idx )
     {
     case 0:
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 49f7c0c..5fd47de 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -471,6 +471,13 @@ unsigned long pv_guest_cr4_fixup(const struct vcpu *, unsigned long guest_cr4);
     ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD |      \
              X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE))
 
+bool_t domain_cpuid_exists(struct domain *d,
+                           unsigned int  input,
+                           unsigned int  sub_input,
+                           unsigned int  *eax,
+                           unsigned int  *ebx,
+                           unsigned int  *ecx,
+                           unsigned int  *edx);
 void domain_cpuid(struct domain *d,
                   unsigned int  input,
                   unsigned int  sub_input,
-- 
1.7.10.4

[PATCH v3 2/4] x86/hvm: Revert 80ecb40362365ba77e68fc609de8bd3b7208ae19

[Boris Ostrovsky]

The Solaris bug that commit 80ecb40362365ba77e68fc609de8bd3b7208ae19
addressed has been fixed and backported to earlier releases.

Those still using those releases can specify number of hypervisor leaves
explicitly via 'cpuid' xl config option.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/traps.c |   11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index e1f39d9..75a0ceb 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -677,17 +677,10 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
     struct domain *d = current->domain;
     /* Optionally shift out of the way of Viridian architectural leaves. */
     uint32_t base = is_viridian_domain(d) ? 0x40000100 : 0x40000000;
-    uint32_t limit;
 
     idx -= base;
 
-    /*
-     * Some Solaris PV drivers fail if max > base + 2. Help them out by
-     * hiding the PVRDTSCP leaf if PVRDTSCP is disabled.
-     */
-    limit = (d->arch.tsc_mode < TSC_MODE_PVRDTSCP) ? 2 : 3;
-
-    if ( idx > limit ) 
+    if ( idx > 3 ) 
         return 0;
 
     if ( domain_cpuid_exists(d, base + idx, sub_idx, eax, ebx, ecx, edx) )
@@ -696,7 +689,7 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
     switch ( idx )
     {
     case 0:
-        *eax = base + limit; /* Largest leaf */
+        *eax = base + 3; /* Largest leaf */
         *ebx = XEN_CPUID_SIGNATURE_EBX;
         *ecx = XEN_CPUID_SIGNATURE_ECX;
         *edx = XEN_CPUID_SIGNATURE_EDX;
-- 
1.7.10.4

[PATCH v3 3/4] x86/hvm: Add HVM-specific hypervisor CPUID leaf

[Boris Ostrovsky]

CPUID leaf 0x40000004 is for HVM-specific features.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/hvm/hvm.c        |   16 ++++++++++++++++
 xen/arch/x86/traps.c          |    8 ++++++--
 xen/include/asm-x86/hvm/hvm.h |    7 +++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ae24211..fcbdcfc 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2980,6 +2980,22 @@ unsigned long copy_from_user_hvm(void *to, const void *from, unsigned len)
     return rc ? len : 0; /* fake a copy_from_user() return code */
 }
 
+void hvm_hypervisor_cpuid_leaf(uint32_t idx, uint32_t sub_idx,
+                               uint32_t *eax, uint32_t *ebx,
+                               uint32_t *ecx, uint32_t *edx)
+{
+    if ( idx != 4 )
+        return;
+
+    *eax = *ebx = *ecx = *edx = 0;
+    if ( !hvm_funcs.hypervisor_cpuid_leaf )
+            return;
+
+    hvm_funcs.hypervisor_cpuid_leaf(idx, sub_idx, eax, ebx, ecx, edx);
+
+    return;
+}
+
 void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
                                    unsigned int *ecx, unsigned int *edx)
 {
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 75a0ceb..88956b8 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -680,7 +680,7 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
 
     idx -= base;
 
-    if ( idx > 3 ) 
+    if ( idx > 4 ) 
         return 0;
 
     if ( domain_cpuid_exists(d, base + idx, sub_idx, eax, ebx, ecx, edx) )
@@ -689,7 +689,7 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
     switch ( idx )
     {
     case 0:
-        *eax = base + 3; /* Largest leaf */
+        *eax = base + 4; /* Largest leaf */
         *ebx = XEN_CPUID_SIGNATURE_EBX;
         *ecx = XEN_CPUID_SIGNATURE_ECX;
         *edx = XEN_CPUID_SIGNATURE_EDX;
@@ -718,6 +718,10 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
         cpuid_time_leaf( sub_idx, eax, ebx, ecx, edx );
         break;
 
+    case 4:
+        hvm_hypervisor_cpuid_leaf(idx, sub_idx, eax, ebx, ecx, edx);
+        break;
+
     default:
         BUG();
     }
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index dcc3483..e6b1399 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -200,6 +200,10 @@ struct hvm_function_table {
                                 paddr_t *L1_gpa, unsigned int *page_order,
                                 uint8_t *p2m_acc, bool_t access_r,
                                 bool_t access_w, bool_t access_x);
+
+    void (*hypervisor_cpuid_leaf)(uint32_t idx, uint32_t sub_idx,
+                                  uint32_t *eax, uint32_t *ebx,
+                                  uint32_t *ecx, uint32_t *edx);
 };
 
 extern struct hvm_function_table hvm_funcs;
@@ -336,6 +340,9 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
 #define is_viridian_domain(_d)                                             \
  (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
 
+void hvm_hypervisor_cpuid_leaf(uint32_t idx, uint32_t sub_idx,
+                               uint32_t *eax, uint32_t *ebx,
+                               uint32_t *ecx, uint32_t *edx);
 void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
                                    unsigned int *ecx, unsigned int *edx);
 void hvm_migrate_timers(struct vcpu *v);
-- 
1.7.10.4

[PATCH v3 4/4] x86/hvm: Indicate avaliability of HW support of APIC virtualization to HVM guests

[Boris Ostrovsky]

Set bits in hypervisor CPUID leaf indicating that HW provides (and the
hypervisor enables) HW support for APIC and x2APIC virtualization.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/hvm/vmx/vmx.c          |   12 ++++++++++++
 xen/include/public/arch-x86/cpuid.h |   10 ++++++++++
 2 files changed, 22 insertions(+)

diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 8395e86..e498c26 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -57,6 +57,7 @@
 #include <asm/apic.h>
 #include <asm/hvm/nestedhvm.h>
 #include <asm/event.h>
+#include <public/arch-x86/cpuid.h>
 
 enum handler_return { HNDL_done, HNDL_unhandled, HNDL_exception_raised };
 
@@ -1646,6 +1647,16 @@ static void vmx_handle_eoi(u8 vector)
     __vmwrite(GUEST_INTR_STATUS, status);
 }
 
+void vmx_hypervisor_cpuid_leaf(uint32_t idx, uint32_t sub_idx,
+                               uint32_t *eax, uint32_t *ebx,
+                               uint32_t *ecx, uint32_t *edx)
+{
+    if ( cpu_has_vmx_apic_reg_virt )
+	    *eax |= XEN_HVM_CPUID_APIC_ACCESS_VIRT;
+    if ( cpu_has_vmx_virtualize_x2apic_mode )
+        *eax |= XEN_HVM_CPUID_X2APIC_VIRT;
+}
+
 static struct hvm_function_table __initdata vmx_function_table = {
     .name                 = "VMX",
     .cpu_up_prepare       = vmx_cpu_up_prepare,
@@ -1703,6 +1714,7 @@ static struct hvm_function_table __initdata vmx_function_table = {
     .sync_pir_to_irr      = vmx_sync_pir_to_irr,
     .handle_eoi           = vmx_handle_eoi,
     .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
+    .hypervisor_cpuid_leaf= vmx_hypervisor_cpuid_leaf,
 };
 
 const struct hvm_function_table * __init start_vmx(void)
diff --git a/xen/include/public/arch-x86/cpuid.h b/xen/include/public/arch-x86/cpuid.h
index d9bd627..7118760 100644
--- a/xen/include/public/arch-x86/cpuid.h
+++ b/xen/include/public/arch-x86/cpuid.h
@@ -65,4 +65,14 @@
 #define _XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD 0
 #define XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD  (1u<<0)
 
+/*
+ * Leaf 5 (0x40000004)
+ * HVM-specific features
+ */
+
+/* EAX Features */
+#define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized APIC registers */
+#define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized x2APIC accesses */
+
+
 #endif /* __XEN_PUBLIC_ARCH_X86_CPUID_H__ */
-- 
1.7.10.4

Re: [PATCH v3 4/4] x86/hvm: Indicate avaliability of HW support of APIC virtualization to HVM guests

[Zhang, Yang Z]

Boris Ostrovsky wrote on 2014-03-14:
> Set bits in hypervisor CPUID leaf indicating that HW provides (and the
> hypervisor enables) HW support for APIC and x2APIC virtualization.
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  xen/arch/x86/hvm/vmx/vmx.c          |   12 ++++++++++++
>  xen/include/public/arch-x86/cpuid.h |   10 ++++++++++
>  2 files changed, 22 insertions(+)
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 8395e86..e498c26 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++
> b/xen/arch/x86/hvm/vmx/vmx.c @@ -57,6 +57,7 @@
>  #include <asm/apic.h>
>  #include <asm/hvm/nestedhvm.h>
>  #include <asm/event.h>
> +#include <public/arch-x86/cpuid.h>
> 
>  enum handler_return { HNDL_done, HNDL_unhandled,
> HNDL_exception_raised };
> 
> @@ -1646,6 +1647,16 @@ static void vmx_handle_eoi(u8 vector)
>      __vmwrite(GUEST_INTR_STATUS, status);  }
> +void vmx_hypervisor_cpuid_leaf(uint32_t idx, uint32_t sub_idx,
> +                               uint32_t *eax, uint32_t *ebx,
> +                               uint32_t *ecx, uint32_t *edx) {
> +    if ( cpu_has_vmx_apic_reg_virt )
> +	    *eax |= XEN_HVM_CPUID_APIC_ACCESS_VIRT;
> +    if ( cpu_has_vmx_virtualize_x2apic_mode )
> +        *eax |= XEN_HVM_CPUID_X2APIC_VIRT; }
> +
>  static struct hvm_function_table __initdata vmx_function_table = {
>      .name                 = "VMX",
>      .cpu_up_prepare       = vmx_cpu_up_prepare,
> @@ -1703,6 +1714,7 @@ static struct hvm_function_table __initdata
> vmx_function_table = {
>      .sync_pir_to_irr      = vmx_sync_pir_to_irr,
>      .handle_eoi           = vmx_handle_eoi,
>      .nhvm_hap_walk_L1_p2m = nvmx_hap_walk_L1_p2m,
> +    .hypervisor_cpuid_leaf= vmx_hypervisor_cpuid_leaf,
>  };
>  
>  const struct hvm_function_table * __init start_vmx(void) diff --git
> a/xen/include/public/arch-x86/cpuid.h
> b/xen/include/public/arch-x86/cpuid.h index d9bd627..7118760 100644 ---
> a/xen/include/public/arch-x86/cpuid.h +++
> b/xen/include/public/arch-x86/cpuid.h @@ -65,4 +65,14 @@
>  #define _XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD 0  #define
> XEN_CPUID_FEAT1_MMU_PT_UPDATE_PRESERVE_AD  (1u<<0)
> 
> +/*
> + * Leaf 5 (0x40000004)
> + * HVM-specific features
> + */
> +
> +/* EAX Features */
> +#define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized APIC
> registers */
> +#define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized
> x2APIC accesses */

I still wonder why expose the x2APIC? I guess you only want to know whether the APICv is used and all platforms that support APICv must also support virtualized x2apic. What can guest do if he knows there is only virtuazliaed x2apic but no APICv?

> +
> +
>  #endif /* __XEN_PUBLIC_ARCH_X86_CPUID_H__ */


Best regards,
Yang

Re: [PATCH v3 1/4] xen/libxc: Allow changes to hypervisor CPUID leaf from config file

[Jan Beulich]

>>> On 13.03.14 at 19:08, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> @@ -43,22 +45,29 @@ static int hypervisor_is_64bit(xc_interface *xch)
>  static void cpuid(const unsigned int *input, unsigned int *regs)
>  {
>      unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1];
> +    uint8_t is_hyp = IS_HYPERVISOR_LEAF(input[0]);
>  #ifdef __i386__
>      /* Use the stack to avoid reg constraint failures with some gcc flags */
>      asm (
>          "push %%ebx; push %%edx\n\t"
> -        "cpuid\n\t"
> +        "testb $0xff,%5\n\t"
> +        "jz 1f\n\t"
> +        XEN_EMULATE_PREFIX
> +        "1: cpuid\n\t"
>          "mov %%ebx,4(%4)\n\t"
>          "mov %%edx,12(%4)\n\t"
>          "pop %%edx; pop %%ebx\n\t"
>          : "=a" (regs[0]), "=c" (regs[2])
> -        : "0" (input[0]), "1" (count), "S" (regs)
> +        : "0" (input[0]), "1" (count), "S" (regs), "m" (is_hyp)

All inputs must be in registers here, since memory references might
use %esp and hence be off by 2 stack slots due to the pushes/pops
surrounding the actual operation. Since you evaluate the flag prior
to the CPUID, using "db" as constraint would seem possible here.

And I'd additionally recommend stopping the practice of using
numeric labels in inline assembly - especially when nesting things
through multiple macro layers (admittedly not the case here) this
is just too fragile, and gcc has a nice solution ("%=") available.

>          : "memory" );
>  #else
>      asm (
> -        "cpuid"
> +        "testb $0xff,%6\n\t"
> +        "jz 1f\n\t"
> +        XEN_EMULATE_PREFIX
> +        "1: cpuid"
>          : "=a" (regs[0]), "=b" (regs[1]), "=c" (regs[2]), "=d" (regs[3])
> -        : "0" (input[0]), "2" (count) );
> +        : "0" (input[0]), "2" (count), "m" (is_hyp) );

And afaict there's no reason not to use "g" here.

> @@ -555,6 +564,15 @@ static int xc_cpuid_policy(
>  {
>      xc_dominfo_t        info;
>  
> +    if ( IS_HYPERVISOR_LEAF(input[0]) )
> +    {
> +        /* Only leaf 1 can be modified */
> +        if ( input[0] == 0x40000000 )
> +            return 0;
> +        else
> +            return -EACCES;

And I'm still worried about altering this in uncontrolled ways: No
good can result from allowing ecx/edx/ebx to be modified, and
improperly modifying the eax value (namely putting in place
wrong upper bits, the more that those aren't statically
determined) won't help much either. IOW it should really only
be the low 8 bits of eax that can be overridden, and it shouldn't
be possible to clear these 8 bits.

Jan

Re: [PATCH v3 3/4] x86/hvm: Add HVM-specific hypervisor CPUID leaf

[Jan Beulich]

>>> On 13.03.14 at 19:08, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> +void hvm_hypervisor_cpuid_leaf(uint32_t idx, uint32_t sub_idx,
> +                               uint32_t *eax, uint32_t *ebx,
> +                               uint32_t *ecx, uint32_t *edx)
> +{
> +    if ( idx != 4 )
> +        return;

What's the point of this check? Why is "idx" being passed in here in
the first place? With you making use of "sub_idx", there's absolutely
no reason to expect the need for another leaf to ever get funneled
into here.

> +
> +    *eax = *ebx = *ecx = *edx = 0;
> +    if ( !hvm_funcs.hypervisor_cpuid_leaf )
> +            return;
> +
> +    hvm_funcs.hypervisor_cpuid_leaf(idx, sub_idx, eax, ebx, ecx, edx);
> +
> +    return;

Please invert the condition and drop both return-s, at once fixing
the wrong indentation above.

Jan

Re: [PATCH v3 4/4] x86/hvm: Indicate avaliability of HW support of APIC virtualization to HVM guests

[Jan Beulich]

>>> On 13.03.14 at 19:08, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> +void vmx_hypervisor_cpuid_leaf(uint32_t idx, uint32_t sub_idx,
> +                               uint32_t *eax, uint32_t *ebx,
> +                               uint32_t *ecx, uint32_t *edx)
> +{

Need to bail on sub_idx != 0.

> +    if ( cpu_has_vmx_apic_reg_virt )
> +	    *eax |= XEN_HVM_CPUID_APIC_ACCESS_VIRT;

Hard tab.

Jan

Re: [PATCH v3 4/4] x86/hvm: Indicate avaliability of HW support of APIC virtualization to HVM guests

[Boris Ostrovsky]

On 03/13/2014 09:48 PM, Zhang, Yang Z wrote:
>
>
> +/*
> + * Leaf 5 (0x40000004)
> + * HVM-specific features
> + */
> +
> +/* EAX Features */
> +#define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized APIC
> registers */
> +#define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized
> x2APIC accesses */
> I still wonder why expose the x2APIC? I guess you only want to know whether the APICv is used and all platforms that support APICv must also support virtualized x2apic. What can guest do if he knows there is only virtuazliaed x2apic but no APICv?

You mean if it has virtualized x2APIC but not virtualized APIC registers 
(i.e. bit 4 in VMCS secondary controls is set but bit 8 is not)? I 
thought that term APICv encompasses both of these features.

Then it's up to the guest to decide whether to use APIC or pirqs:

* If the guest is in APIC mode then it probably should stick to pirqs 
since memory-mapped accesses to APIC will cause VMEXIT.
* If it is in x2APIC mode then it makes sense for it to not use pirqs 
and go with APIC (x2APIC, really) since accesses will be handled 
transparently.

I said in the cover letter I am not particularly happy about having two 
bits for this but I thought it is justified in this case.

-boris

Re: [PATCH v3 3/4] x86/hvm: Add HVM-specific hypervisor CPUID leaf

[Boris Ostrovsky]

On 03/14/2014 04:14 AM, Jan Beulich wrote:
>>>> On 13.03.14 at 19:08, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> +void hvm_hypervisor_cpuid_leaf(uint32_t idx, uint32_t sub_idx,
>> +                               uint32_t *eax, uint32_t *ebx,
>> +                               uint32_t *ecx, uint32_t *edx)
>> +{
>> +    if ( idx != 4 )
>> +        return;
> What's the point of this check?

Indeed unnecessary, this is already checked at the caller.

> Why is "idx" being passed in here in
> the first place? With you making use of "sub_idx", there's absolutely
> no reason to expect the need for another leaf to ever get funneled
> into here.

I am passing the arguments directly from cpuid_hypervisor_leaves() which
already has idx and sub_idx and I wasn't sure I can assume that they will be
in eax and ecx (which they currently are).

As for sub_idx, I thought that at some point in the future we we might 
use it
so I kept it. But I can drop it since it's rather unlikely.


-boris

Re: [PATCH v3 3/4] x86/hvm: Add HVM-specific hypervisor CPUID leaf

[Jan Beulich]

>>> On 14.03.14 at 15:41, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> On 03/14/2014 04:14 AM, Jan Beulich wrote:
>>>>> On 13.03.14 at 19:08, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>>> +void hvm_hypervisor_cpuid_leaf(uint32_t idx, uint32_t sub_idx,
>>> +                               uint32_t *eax, uint32_t *ebx,
>>> +                               uint32_t *ecx, uint32_t *edx)
>>> +{
>>> +    if ( idx != 4 )
>>> +        return;
>> What's the point of this check?
> 
> Indeed unnecessary, this is already checked at the caller.
> 
>> Why is "idx" being passed in here in
>> the first place? With you making use of "sub_idx", there's absolutely
>> no reason to expect the need for another leaf to ever get funneled
>> into here.
> 
> I am passing the arguments directly from cpuid_hypervisor_leaves() which
> already has idx and sub_idx and I wasn't sure I can assume that they will be
> in eax and ecx (which they currently are).
> 
> As for sub_idx, I thought that at some point in the future we we might 
> use it
> so I kept it. But I can drop it since it's rather unlikely.

You didn't understand me then: I'd like you to drop "idx", and not
because the value is available elsewhere (and I don't think *eax
and *ecx have anything other than zeros), but because you're
passing a value that can only ever be 4. And I didn't ask you to
drop sub_idx at all, but rather to check that it's zero.

Jan

Re: [PATCH v3 1/4] xen/libxc: Allow changes to hypervisor CPUID leaf from config file

[Boris Ostrovsky]

On 03/14/2014 04:09 AM, Jan Beulich wrote:
>>>> On 13.03.14 at 19:08, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> @@ -43,22 +45,29 @@ static int hypervisor_is_64bit(xc_interface *xch)
>>   static void cpuid(const unsigned int *input, unsigned int *regs)
>>   {
>>       unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : input[1];
>> +    uint8_t is_hyp = IS_HYPERVISOR_LEAF(input[0]);
>>   #ifdef __i386__
>>       /* Use the stack to avoid reg constraint failures with some gcc flags */
>>       asm (
>>           "push %%ebx; push %%edx\n\t"
>> -        "cpuid\n\t"
>> +        "testb $0xff,%5\n\t"
>> +        "jz 1f\n\t"
>> +        XEN_EMULATE_PREFIX
>> +        "1: cpuid\n\t"
>>           "mov %%ebx,4(%4)\n\t"
>>           "mov %%edx,12(%4)\n\t"
>>           "pop %%edx; pop %%ebx\n\t"
>>           : "=a" (regs[0]), "=c" (regs[2])
>> -        : "0" (input[0]), "1" (count), "S" (regs)
>> +        : "0" (input[0]), "1" (count), "S" (regs), "m" (is_hyp)
> All inputs must be in registers here, since memory references might
> use %esp and hence be off by 2 stack slots due to the pushes/pops
> surrounding the actual operation. Since you evaluate the flag prior
> to the CPUID, using "db" as constraint would seem possible here.

gcc for some reason rejects "b" as inconsistent (but "d" works fine).


>> @@ -555,6 +564,15 @@ static int xc_cpuid_policy(
>>   {
>>       xc_dominfo_t        info;
>>   
>> +    if ( IS_HYPERVISOR_LEAF(input[0]) )
>> +    {
>> +        /* Only leaf 1 can be modified */
>> +        if ( input[0] == 0x40000000 )
>> +            return 0;
>> +        else
>> +            return -EACCES;
> And I'm still worried about altering this in uncontrolled ways: No
> good can result from allowing ecx/edx/ebx to be modified, and
> improperly modifying the eax value (namely putting in place
> wrong upper bits, the more that those aren't statically
> determined) won't help much either. IOW it should really only
> be the low 8 bits of eax that can be overridden, and it shouldn't
> be possible to clear these 8 bits.

I'll add mask array to xc_cpuid_policy (and probably other policy
routines that it calls) that will specify bits that are permitted to be
overwritten.

-boris

Re: [PATCH v3 1/4] xen/libxc: Allow changes to hypervisor CPUID leaf from config file

[Jan Beulich]

>>> On 14.03.14 at 16:41, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> On 03/14/2014 04:09 AM, Jan Beulich wrote:
>>>>> On 13.03.14 at 19:08, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>>> @@ -43,22 +45,29 @@ static int hypervisor_is_64bit(xc_interface *xch)
>>>   static void cpuid(const unsigned int *input, unsigned int *regs)
>>>   {
>>>       unsigned int count = (input[1] == XEN_CPUID_INPUT_UNUSED) ? 0 : 
> input[1];
>>> +    uint8_t is_hyp = IS_HYPERVISOR_LEAF(input[0]);
>>>   #ifdef __i386__
>>>       /* Use the stack to avoid reg constraint failures with some gcc flags 
> */
>>>       asm (
>>>           "push %%ebx; push %%edx\n\t"
>>> -        "cpuid\n\t"
>>> +        "testb $0xff,%5\n\t"
>>> +        "jz 1f\n\t"
>>> +        XEN_EMULATE_PREFIX
>>> +        "1: cpuid\n\t"
>>>           "mov %%ebx,4(%4)\n\t"
>>>           "mov %%edx,12(%4)\n\t"
>>>           "pop %%edx; pop %%ebx\n\t"
>>>           : "=a" (regs[0]), "=c" (regs[2])
>>> -        : "0" (input[0]), "1" (count), "S" (regs)
>>> +        : "0" (input[0]), "1" (count), "S" (regs), "m" (is_hyp)
>> All inputs must be in registers here, since memory references might
>> use %esp and hence be off by 2 stack slots due to the pushes/pops
>> surrounding the actual operation. Since you evaluate the flag prior
>> to the CPUID, using "db" as constraint would seem possible here.
> 
> gcc for some reason rejects "b" as inconsistent (but "d" works fine).

And I guess "q" or "Q" could be it, then.

Jan

Re: [PATCH v3 4/4] x86/hvm: Indicate avaliability of HW support of APIC virtualization to HVM guests

[Zhang, Yang Z]

Boris Ostrovsky wrote on 2014-03-14:
> On 03/13/2014 09:48 PM, Zhang, Yang Z wrote:
>> 
>> 
>> +/*
>> + * Leaf 5 (0x40000004)
>> + * HVM-specific features
>> + */
>> +
>> +/* EAX Features */
>> +#define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized
>> +APIC
>> registers */
>> +#define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized
>> x2APIC accesses */
>> I still wonder why expose the x2APIC? I guess you only want to know
>> whether
> the APICv is used and all platforms that support APICv must also
> support virtualized x2apic. What can guest do if he knows there is
> only virtuazliaed x2apic but no APICv?
> 
> You mean if it has virtualized x2APIC but not virtualized APIC
> registers (i.e. bit 4 in VMCS secondary controls is set but bit 8 is
> not)? I thought that term APICv encompasses both of these features.
> 
> Then it's up to the guest to decide whether to use APIC or pirqs:
> 
> * If the guest is in APIC mode then it probably should stick to pirqs
> since memory-mapped accesses to APIC will cause VMEXIT.
> * If it is in x2APIC mode then it makes sense for it to not use pirqs
> and go with APIC (x2APIC, really) since accesses will be handled transparently.
> 

Does x2apic mode faster than pirq?

> I said in the cover letter I am not particularly happy about having
> two bits for this but I thought it is justified in this case.
> 
> -boris


Best regards,
Yang

Re: [PATCH v3 4/4] x86/hvm: Indicate avaliability of HW support of APIC virtualization to HVM guests

[Boris Ostrovsky]

On 03/16/2014 08:40 PM, Zhang, Yang Z wrote:
> Boris Ostrovsky wrote on 2014-03-14:
>> On 03/13/2014 09:48 PM, Zhang, Yang Z wrote:
>>>
>>> +/*
>>> + * Leaf 5 (0x40000004)
>>> + * HVM-specific features
>>> + */
>>> +
>>> +/* EAX Features */
>>> +#define XEN_HVM_CPUID_APIC_ACCESS_VIRT (1u << 0) /* Virtualized
>>> +APIC
>>> registers */
>>> +#define XEN_HVM_CPUID_X2APIC_VIRT      (1u << 1) /* Virtualized
>>> x2APIC accesses */
>>> I still wonder why expose the x2APIC? I guess you only want to know
>>> whether
>> the APICv is used and all platforms that support APICv must also
>> support virtualized x2apic. What can guest do if he knows there is
>> only virtuazliaed x2apic but no APICv?
>>
>> You mean if it has virtualized x2APIC but not virtualized APIC
>> registers (i.e. bit 4 in VMCS secondary controls is set but bit 8 is
>> not)? I thought that term APICv encompasses both of these features.
>>
>> Then it's up to the guest to decide whether to use APIC or pirqs:
>>
>> * If the guest is in APIC mode then it probably should stick to pirqs
>> since memory-mapped accesses to APIC will cause VMEXIT.
>> * If it is in x2APIC mode then it makes sense for it to not use pirqs
>> and go with APIC (x2APIC, really) since accesses will be handled transparently.
>>
> Does x2apic mode faster than pirq?

In terms of the number of VMEXITs -- yes, it results in far fewer of 
them. For example, eoi_pirq() may end up in a hypercall, something that 
will be avoided with virtualized x2APIC.


-boris

>
>> I said in the cover letter I am not particularly happy about having
>> two bits for this but I thought it is justified in this case.
>>
>> -boris
>
> Best regards,
> Yang
>
>