[PATCH 0/6] AMD-PVH: DomU support

[PATCH 0/6] AMD-PVH: DomU support

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

This is a re-spin of patches for AMD PVH DomU from Mukesh Rathor.
As I am diving into more details of AMD PVH, I am reposting his series
with minor changes that reviewers (Jan and Boris) posted in comments.

The issue with handle_mmio is not yet addressed and I would like to
continue discussion Mukesh and Jan previously had in this thread
http://lists.xen.org/archives/html/xen-devel/2014-08/msg01760.html
The latest proposed solution was to create additional x86_emulate_ops structure
that will handle pvh mmio correctly.
Should I consider this approach as the one I should be working on?

In vmcb construction patch comments Roger suggested to add additional
parameter to vcpu_initialise as 32 bit work is in. Since Boris has 
posted 32-bit pvh domU support, that would be changed and I wanted to
see if this is what everyone agrees on.

Any other ideas/comments are also appreciated.
Thank you.

Changes made in this re-post:
 - left out setting LMA bit in construct_vmcb as its done in hvm_vcpu_initialise;
 - instead of checking if regs ptr is set in vcpu_vlapic, check if its pvh_domain;


Elena Ufimtseva (6):
  pvh: domu construct vmcb 64 bit mode start
  AMD-PVH: cpuid intercept
  AMD-PVH: call hvm_emulate_one instead of handle_mmio
  AMD-PVH: Do not get/set vlapic TPR
  AMD-PVH: Support TSC_MODE_NEVER_EMULATE for PVH
  AMD-PVH: enable pvh if requirements met

 xen/arch/x86/hvm/svm/svm.c  | 80 ++++++++++++++++++++++++++++++---------------
 xen/arch/x86/hvm/svm/vmcb.c | 16 +++++++--
 xen/arch/x86/time.c         |  1 +
 3 files changed, 68 insertions(+), 29 deletions(-)

-- 
1.9.3

[PATCH 1/6] pvh: domu construct vmcb 64 bit mode start

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/hvm/svm/vmcb.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 6339d2a..70a6588 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -162,7 +162,12 @@ static int construct_vmcb(struct vcpu *v)
     vmcb->ds.attr.bytes = 0xc93;
     vmcb->fs.attr.bytes = 0xc93;
     vmcb->gs.attr.bytes = 0xc93;
-    vmcb->cs.attr.bytes = 0xc9b; /* exec/read, accessed */
+
+    if ( is_pvh_vcpu(v) )
+        /* CS.L == 1, exec, read/write, accessed. PVH 32bitfixme. */
+        vmcb->cs.attr.bytes = 0xa9b;
+    else
+        vmcb->cs.attr.bytes = 0xc9b; /* exec/read, accessed */
 
     /* Guest IDT. */
     vmcb->idtr.base = 0;
@@ -184,12 +189,17 @@ static int construct_vmcb(struct vcpu *v)
     vmcb->tr.limit = 0xff;
 
     v->arch.hvm_vcpu.guest_cr[0] = X86_CR0_PE | X86_CR0_ET;
+    /* PVH domains start in paging mode */
+    if ( is_pvh_vcpu(v) )
+        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG;
     hvm_update_guest_cr(v, 0);
 
-    v->arch.hvm_vcpu.guest_cr[4] = 0;
+    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_vcpu(v) ? X86_CR4_PAE : 0;
     hvm_update_guest_cr(v, 4);
 
-    paging_update_paging_modes(v);
+    /* For pvh, paging mode is updated by arch_set_info_guest(). */
+    if ( is_hvm_vcpu(v) )
+        paging_update_paging_modes(v);
 
     vmcb->_exception_intercepts =
         HVM_TRAP_MASK
-- 
1.9.3

[PATCH 2/6] AMD-PVH: cpuid intercept

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

Call pv_cpuid for pvh cpuid intercept. Note, we modify
svm_vmexit_do_cpuid instead of the intercept switch because the guest
eip needs to be adjusted for pvh also.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/hvm/svm/svm.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 6734fb6..28792fe 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1584,19 +1584,22 @@ static void svm_vmexit_do_cpuid(struct cpu_user_regs *regs)
 
     if ( (inst_len = __get_instruction_length(current, INSTR_CPUID)) == 0 )
         return;
+    if ( is_pvh_vcpu(current) )
+        pv_cpuid(regs);
+    else
+    {
+        eax = regs->eax;
+        ebx = regs->ebx;
+        ecx = regs->ecx;
+        edx = regs->edx;
 
-    eax = regs->eax;
-    ebx = regs->ebx;
-    ecx = regs->ecx;
-    edx = regs->edx;
-
-    svm_cpuid_intercept(&eax, &ebx, &ecx, &edx);
-
-    regs->eax = eax;
-    regs->ebx = ebx;
-    regs->ecx = ecx;
-    regs->edx = edx;
+        svm_cpuid_intercept(&eax, &ebx, &ecx, &edx);
 
+        regs->eax = eax;
+        regs->ebx = ebx;
+        regs->ecx = ecx;
+        regs->edx = edx;
+    }
     __update_guest_eip(regs, inst_len);
 }
 
-- 
1.9.3

[PATCH 3/6] AMD-PVH: call hvm_emulate_one instead of handle_mmio

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

Certain IOIO instructions and CR access instructions like
lmsw/clts etc need to be emulated. handle_mmio is incorrectly called to
accomplish this. Create svm_emulate() to call hvm_emulate_one which is more
appropriate, and works for pvh as well. handle_mmio call is
forbidden for pvh.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/hvm/svm/svm.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 28792fe..e7262c9 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2289,6 +2289,23 @@ static struct hvm_function_table __initdata svm_function_table = {
     .nhvm_hap_walk_L1_p2m = nsvm_hap_walk_L1_p2m,
 };
 
+static void svm_emulate(struct cpu_user_regs *regs)
+{
+    int rc;
+    struct hvm_emulate_ctxt ctxt;
+
+    hvm_emulate_prepare(&ctxt, regs);
+    rc = hvm_emulate_one(&ctxt);
+
+    if ( rc != X86EMUL_OKAY )
+    {
+	if ( ctxt.exn_pending )
+		hvm_inject_trap(&ctxt.trap);
+	else
+		hvm_inject_hw_exception(TRAP_gp_fault, 0);
+    }
+}
+
 void svm_vmexit_handler(struct cpu_user_regs *regs)
 {
     uint64_t exit_reason;
@@ -2555,16 +2572,16 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
             if ( handle_pio(port, bytes, dir) )
                 __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
         }
-        else if ( !handle_mmio() )
-            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+        else
+            svm_emulate(regs);
         break;
 
     case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
     case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
         if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) )
             svm_vmexit_do_cr_access(vmcb, regs);
-        else if ( !handle_mmio() ) 
-            hvm_inject_hw_exception(TRAP_gp_fault, 0);
+        else
+            svm_emulate(regs);
         break;
 
     case VMEXIT_INVLPG:
@@ -2575,6 +2592,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
         }
         else if ( !handle_mmio() )
             hvm_inject_hw_exception(TRAP_gp_fault, 0);
+	else
+            svm_emulate(regs);
         break;
 
     case VMEXIT_INVLPGA:
-- 
1.9.3

[PATCH 4/6] AMD-PVH: Do not get/set vlapic TPR

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

PVH doesn't use apic emulation hence vlapic->regs ptr is not set for it.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/hvm/svm/svm.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index e7262c9..64d22fe 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1059,7 +1059,7 @@ static void noreturn svm_do_resume(struct vcpu *v)
         hvm_asid_flush_vcpu(v);
     }
 
-    if ( !vcpu_guestmode )
+    if ( !vcpu_guestmode && !is_pvh_domain(v->domain) )
     {
         vintr_t intr;
 
@@ -2332,7 +2332,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
      * NB. We need to preserve the low bits of the TPR to make checked builds
      * of Windows work, even though they don't actually do anything.
      */
-    if ( !vcpu_guestmode ) {
+    if ( !vcpu_guestmode && !is_pvh_domain(v->domain) ) {
         intr = vmcb_get_vintr(vmcb);
         vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI,
                    ((intr.fields.tpr & 0x0F) << 4) |
@@ -2720,15 +2720,18 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
     }
 
   out:
-    if ( vcpu_guestmode )
-        /* Don't clobber TPR of the nested guest. */
-        return;
-
-    /* The exit may have updated the TPR: reflect this in the hardware vtpr */
-    intr = vmcb_get_vintr(vmcb);
-    intr.fields.tpr =
-        (vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xFF) >> 4;
-    vmcb_set_vintr(vmcb, intr);
+    /* Don't clobber TPR of the nested guest. */
+    if ( vcpu_guestmode && !is_pvh_domain(v->domain) )
+    {
+        /*
+         * The exit may have updated the TPR: reflect this in the hardware
+         * vtpr.
+         */
+        intr = vmcb_get_vintr(vmcb);
+        intr.fields.tpr =
+            (vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xFF) >> 4;
+        vmcb_set_vintr(vmcb, intr);
+    }
 }
 
 void svm_trace_vmentry(void)
-- 
1.9.3

[PATCH 5/6] AMD-PVH: Support TSC_MODE_NEVER_EMULATE for PVH

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

On AMD, MSR_AMD64_TSC_RATIO must be set for rdtsc instruction in guest
to properly read the cpu tsc. To that end, set tsc_khz in struct domain.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/time.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index bbb7e6c..d9709ce 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1923,6 +1923,7 @@ void tsc_set_info(struct domain *d,
          * but "always_emulate" does not for some reason.  Figure out
          * why.
          */
+        d->arch.tsc_khz = cpu_khz;
         switch ( tsc_mode )
         {
         case TSC_MODE_NEVER_EMULATE:
-- 
1.9.3

[PATCH 6/6] AMD-PVH: enable pvh if requirements met

[elena.ufimtseva]

From: Elena Ufimtseva <elena.ufimtseva@oracle.com>

Finally, enable pvh if the cpu supports NPT and svm decode.

Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
---
 xen/arch/x86/hvm/svm/svm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 64d22fe..9945550 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1444,6 +1444,9 @@ const struct hvm_function_table * __init start_svm(void)
     svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
         ((cpuid_edx(0x80000001) & 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0);
 
+    if ( cpu_has_svm_npt  && cpu_has_svm_decode )
+        svm_function_table.pvh_supported = 1;
+
     return &svm_function_table;
 }
 
-- 
1.9.3

Re: [PATCH 5/6] AMD-PVH: Support TSC_MODE_NEVER_EMULATE for PVH

[Konrad Rzeszutek Wilk]

On Mon, Jun 22, 2015 at 12:37:37PM -0400, elena.ufimtseva@oracle.com wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> On AMD, MSR_AMD64_TSC_RATIO must be set for rdtsc instruction in guest
> to properly read the cpu tsc. To that end, set tsc_khz in struct domain.

It looks like the TSC_MODE_DEFAULT, TSC_MODE_ALWAYS_EMULATE, and
TSC_MODE_PVRDTSCP already set tsc_khz.

Only TSC_MODE_NEVER_EMULATE hadn't set it (but that
piece of code has some continuation from TSC_MODE_DEFAULT).

And we need this otherwise the CPU tsc offset has bogus data?

Anyway if this is the way to go, you can probably delete:

1968         d->arch.tsc_khz = cpu_khz;                                              

> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  xen/arch/x86/time.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index bbb7e6c..d9709ce 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1923,6 +1923,7 @@ void tsc_set_info(struct domain *d,
>           * but "always_emulate" does not for some reason.  Figure out
>           * why.
>           */
> +        d->arch.tsc_khz = cpu_khz;
>          switch ( tsc_mode )
>          {
>          case TSC_MODE_NEVER_EMULATE:
> -- 
> 1.9.3
> 

Re: [PATCH 1/6] pvh: domu construct vmcb 64 bit mode start

[Boris Ostrovsky]

On 06/22/2015 12:37 PM, elena.ufimtseva@oracle.com wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---

All of these changes are common with Intel so we should be able to move 
them up to common layer. (For CS attributes the 32-bit patches that I 
posted earlier provide set_mode op).

-boris

>   xen/arch/x86/hvm/svm/vmcb.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 6339d2a..70a6588 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -162,7 +162,12 @@ static int construct_vmcb(struct vcpu *v)
>       vmcb->ds.attr.bytes = 0xc93;
>       vmcb->fs.attr.bytes = 0xc93;
>       vmcb->gs.attr.bytes = 0xc93;
> -    vmcb->cs.attr.bytes = 0xc9b; /* exec/read, accessed */
> +
> +    if ( is_pvh_vcpu(v) )
> +        /* CS.L == 1, exec, read/write, accessed. PVH 32bitfixme. */
> +        vmcb->cs.attr.bytes = 0xa9b;
> +    else
> +        vmcb->cs.attr.bytes = 0xc9b; /* exec/read, accessed */
>   
>       /* Guest IDT. */
>       vmcb->idtr.base = 0;
> @@ -184,12 +189,17 @@ static int construct_vmcb(struct vcpu *v)
>       vmcb->tr.limit = 0xff;
>   
>       v->arch.hvm_vcpu.guest_cr[0] = X86_CR0_PE | X86_CR0_ET;
> +    /* PVH domains start in paging mode */
> +    if ( is_pvh_vcpu(v) )
> +        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG;
>       hvm_update_guest_cr(v, 0);
>   
> -    v->arch.hvm_vcpu.guest_cr[4] = 0;
> +    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_vcpu(v) ? X86_CR4_PAE : 0;
>       hvm_update_guest_cr(v, 4);
>   
> -    paging_update_paging_modes(v);
> +    /* For pvh, paging mode is updated by arch_set_info_guest(). */
> +    if ( is_hvm_vcpu(v) )
> +        paging_update_paging_modes(v);
>   
>       vmcb->_exception_intercepts =
>           HVM_TRAP_MASK

Re: [PATCH 3/6] AMD-PVH: call hvm_emulate_one instead of handle_mmio

[Boris Ostrovsky]

On 06/22/2015 12:37 PM, elena.ufimtseva@oracle.com wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>
> Certain IOIO instructions and CR access instructions like
> lmsw/clts etc need to be emulated. handle_mmio is incorrectly called to
> accomplish this. Create svm_emulate() to call hvm_emulate_one which is more
> appropriate, and works for pvh as well. handle_mmio call is
> forbidden for pvh.
>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>   xen/arch/x86/hvm/svm/svm.c | 27 +++++++++++++++++++++++----
>   1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 28792fe..e7262c9 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2289,6 +2289,23 @@ static struct hvm_function_table __initdata svm_function_table = {
>       .nhvm_hap_walk_L1_p2m = nsvm_hap_walk_L1_p2m,
>   };
>   
> +static void svm_emulate(struct cpu_user_regs *regs)
> +{
> +    int rc;
> +    struct hvm_emulate_ctxt ctxt;
> +
> +    hvm_emulate_prepare(&ctxt, regs);
> +    rc = hvm_emulate_one(&ctxt);
> +
> +    if ( rc != X86EMUL_OKAY )
> +    {
> +	if ( ctxt.exn_pending )
> +		hvm_inject_trap(&ctxt.trap);
> +	else
> +		hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +    }
> +}
> +
>   void svm_vmexit_handler(struct cpu_user_regs *regs)
>   {
>       uint64_t exit_reason;
> @@ -2555,16 +2572,16 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>               if ( handle_pio(port, bytes, dir) )
>                   __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
>           }
> -        else if ( !handle_mmio() )
> -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +        else
> +            svm_emulate(regs);
>           break;
>   
>       case VMEXIT_CR0_READ ... VMEXIT_CR15_READ:
>       case VMEXIT_CR0_WRITE ... VMEXIT_CR15_WRITE:
>           if ( cpu_has_svm_decode && (vmcb->exitinfo1 & (1ULL << 63)) )
>               svm_vmexit_do_cr_access(vmcb, regs);
> -        else if ( !handle_mmio() )
> -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +        else
> +            svm_emulate(regs);
>           break;
>   
>       case VMEXIT_INVLPG:
> @@ -2575,6 +2592,8 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>           }
>           else if ( !handle_mmio() )
>               hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +	else
> +            svm_emulate(regs);
>           break;
>   
>       case VMEXIT_INVLPGA:


handle_mmio() will pop the assertion on PVH 
('ASSERT(!is_pvh_vcpu(curr))'). I think just calling svm_emulate() 
should be sufficient, just like it is for CR registers.

-boris

Re: [PATCH 0/6] AMD-PVH: DomU support

[Jan Beulich]

>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> The issue with handle_mmio is not yet addressed and I would like to
> continue discussion Mukesh and Jan previously had in this thread
> http://lists.xen.org/archives/html/xen-devel/2014-08/msg01760.html 
> The latest proposed solution was to create additional x86_emulate_ops 
> structure
> that will handle pvh mmio correctly.
> Should I consider this approach as the one I should be working on?

Yes, I think so, unless you see a better alternative.

Jan

Re: [PATCH 1/6] pvh: domu construct vmcb 64 bit mode start

[Jan Beulich]

>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>

As long as this patch originally cam from Mukesh, From: should
reflect that imo. Once you made changes, you S-o-b would be
required alongside his.

> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -162,7 +162,12 @@ static int construct_vmcb(struct vcpu *v)
>      vmcb->ds.attr.bytes = 0xc93;
>      vmcb->fs.attr.bytes = 0xc93;
>      vmcb->gs.attr.bytes = 0xc93;
> -    vmcb->cs.attr.bytes = 0xc9b; /* exec/read, accessed */
> +
> +    if ( is_pvh_vcpu(v) )
> +        /* CS.L == 1, exec, read/write, accessed. PVH 32bitfixme. */
> +        vmcb->cs.attr.bytes = 0xa9b;
> +    else
> +        vmcb->cs.attr.bytes = 0xc9b; /* exec/read, accessed */

With 32-bit support now actively being worked on, I don't think
we want to see any new 32bitfixme-s proposed to go in. Plus it
needs settling on whether the boot mode is to change for PVH.

Jan

Re: [PATCH 2/6] AMD-PVH: cpuid intercept

[Jan Beulich]

>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> 
> Call pv_cpuid for pvh cpuid intercept. Note, we modify
> svm_vmexit_do_cpuid instead of the intercept switch because the guest
> eip needs to be adjusted for pvh also.

But that doesn't preclude the adjustment to be done in
svm_cpuid_intercept(), and it needs to be there as it's also used
as the implementation of .cpuid_intercept.

Jan

Re: [PATCH 3/6] AMD-PVH: call hvm_emulate_one instead of handle_mmio

[Jan Beulich]

>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2289,6 +2289,23 @@ static struct hvm_function_table __initdata svm_function_table = {
>      .nhvm_hap_walk_L1_p2m = nsvm_hap_walk_L1_p2m,
>  };
>  
> +static void svm_emulate(struct cpu_user_regs *regs)
> +{
> +    int rc;
> +    struct hvm_emulate_ctxt ctxt;
> +
> +    hvm_emulate_prepare(&ctxt, regs);
> +    rc = hvm_emulate_one(&ctxt);
> +
> +    if ( rc != X86EMUL_OKAY )
> +    {
> +	if ( ctxt.exn_pending )
> +		hvm_inject_trap(&ctxt.trap);
> +	else
> +		hvm_inject_hw_exception(TRAP_gp_fault, 0);

Indentation. Also I think this might better be

    if ( ctxt.exn_pending )
    ...
    else if ( rc != X86EMUL_OKAY )
    ...

> @@ -2555,16 +2572,16 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>              if ( handle_pio(port, bytes, dir) )
>                  __update_guest_eip(regs, vmcb->exitinfo2 - vmcb->rip);
>          }
> -        else if ( !handle_mmio() )
> -            hvm_inject_hw_exception(TRAP_gp_fault, 0);
> +        else
> +            svm_emulate(regs);

As said in the original thread, this can't be done this way (not the
least because it then also affects HVM, where handle_mmio()
should be used when you can't tell up front that no memory is
being accessed.

Jan

Re: [PATCH 4/6] AMD-PVH: Do not get/set vlapic TPR

[Jan Beulich]

>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> @@ -2720,15 +2720,18 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>      }
>  
>    out:
> -    if ( vcpu_guestmode )

Why can't you just adjust this if() and leave the rest of the code alone?

Jan

> -        /* Don't clobber TPR of the nested guest. */
> -        return;
> -
> -    /* The exit may have updated the TPR: reflect this in the hardware vtpr */
> -    intr = vmcb_get_vintr(vmcb);
> -    intr.fields.tpr =
> -        (vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xFF) >> 4;
> -    vmcb_set_vintr(vmcb, intr);
> +    /* Don't clobber TPR of the nested guest. */
> +    if ( vcpu_guestmode && !is_pvh_domain(v->domain) )
> +    {
> +        /*
> +         * The exit may have updated the TPR: reflect this in the hardware
> +         * vtpr.
> +         */
> +        intr = vmcb_get_vintr(vmcb);
> +        intr.fields.tpr =
> +            (vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI) & 0xFF) >> 4;
> +        vmcb_set_vintr(vmcb, intr);
> +    }
>  }
>  
>  void svm_trace_vmentry(void)
> -- 
> 1.9.3

Re: [PATCH 5/6] AMD-PVH: Support TSC_MODE_NEVER_EMULATE for PVH

[Jan Beulich]

>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1923,6 +1923,7 @@ void tsc_set_info(struct domain *d,
>           * but "always_emulate" does not for some reason.  Figure out
>           * why.
>           */
> +        d->arch.tsc_khz = cpu_khz;
>          switch ( tsc_mode )
>          {
>          case TSC_MODE_NEVER_EMULATE:

Considering that it's vcpu_tsc_ratio() that wants this set, I don't
think doing this here (and hence depending on this function being
called early enough / at all) is appropriate. Instead I think this
should be done in SVM's vCPU initialization.

Jan

Re: [PATCH 6/6] AMD-PVH: enable pvh if requirements met

[Jan Beulich]

>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1444,6 +1444,9 @@ const struct hvm_function_table * __init 
> start_svm(void)
>      svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
>          ((cpuid_edx(0x80000001) & 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0);
>  
> +    if ( cpu_has_svm_npt  && cpu_has_svm_decode )
> +        svm_function_table.pvh_supported = 1;

If svm_decode indeed is a prereq, then the earlier patch dealing
with the handle_mmio() invocations doesn't need to fiddle with
VMEXIT_INVLPG other than to maybe add a documenting ASSERT().

Jan

Re: [PATCH 6/6] AMD-PVH: enable pvh if requirements met

[Boris Ostrovsky]

On 06/23/2015 08:30 AM, Jan Beulich wrote:
>>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1444,6 +1444,9 @@ const struct hvm_function_table * __init
>> start_svm(void)
>>       svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
>>           ((cpuid_edx(0x80000001) & 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0);
>>   
>> +    if ( cpu_has_svm_npt  && cpu_has_svm_decode )
>> +        svm_function_table.pvh_supported = 1;
> If svm_decode indeed is a prereq, then the earlier patch dealing
> with the handle_mmio() invocations doesn't need to fiddle with
> VMEXIT_INVLPG other than to maybe add a documenting ASSERT().
>

I am not sure we should require decode feature to be required for PVH 
support. I can't remember exactly but I think this feature was first 
introduced in family 15h so requiring it will leave at least family 10h 
processors as not supporting PVH.

-boris

Re: [PATCH 6/6] AMD-PVH: enable pvh if requirements met

[Jan Beulich]

>>> On 24.06.15 at 04:34, <boris.ostrovsky@oracle.com> wrote:
> On 06/23/2015 08:30 AM, Jan Beulich wrote:
>>>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -1444,6 +1444,9 @@ const struct hvm_function_table * __init
>>> start_svm(void)
>>>       svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
>>>           ((cpuid_edx(0x80000001) & 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0);
>>>   
>>> +    if ( cpu_has_svm_npt  && cpu_has_svm_decode )
>>> +        svm_function_table.pvh_supported = 1;
>> If svm_decode indeed is a prereq, then the earlier patch dealing
>> with the handle_mmio() invocations doesn't need to fiddle with
>> VMEXIT_INVLPG other than to maybe add a documenting ASSERT().
>>
> 
> I am not sure we should require decode feature to be required for PVH 
> support. I can't remember exactly but I think this feature was first 
> introduced in family 15h so requiring it will leave at least family 10h 
> processors as not supporting PVH.

The question was why the dependency was added in the first place.
Indeed only fam 12, 15, and 16 have the field documented. Otoh
PVH isn't being supported universally on all VMX variants either...

Jan

Re: [PATCH 6/6] AMD-PVH: enable pvh if requirements met

[Andrew Cooper]

On 24/06/15 08:49, Jan Beulich wrote:
>>>> On 24.06.15 at 04:34, <boris.ostrovsky@oracle.com> wrote:
>> On 06/23/2015 08:30 AM, Jan Beulich wrote:
>>>>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
>>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>>> @@ -1444,6 +1444,9 @@ const struct hvm_function_table * __init
>>>> start_svm(void)
>>>>       svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
>>>>           ((cpuid_edx(0x80000001) & 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0);
>>>>   
>>>> +    if ( cpu_has_svm_npt  && cpu_has_svm_decode )
>>>> +        svm_function_table.pvh_supported = 1;
>>> If svm_decode indeed is a prereq, then the earlier patch dealing
>>> with the handle_mmio() invocations doesn't need to fiddle with
>>> VMEXIT_INVLPG other than to maybe add a documenting ASSERT().
>>>
>> I am not sure we should require decode feature to be required for PVH 
>> support. I can't remember exactly but I think this feature was first 
>> introduced in family 15h so requiring it will leave at least family 10h 
>> processors as not supporting PVH.
> The question was why the dependency was added in the first place.
> Indeed only fam 12, 15, and 16 have the field documented. Otoh
> PVH isn't being supported universally on all VMX variants either...

Right, but this is a bug (feature?) of the current implementation and
need fixing.

There are no technical reasons to prevent PVH guests running in any case
where an HVM guest currently runs.

The only technical restriction I can think of is that a PVH hardware
domain needs IOMMU support, but that is it.

~Andrew

Re: [PATCH 1/6] pvh: domu construct vmcb 64 bit mode start

[Elena Ufimtseva]

On Tue, Jun 23, 2015 at 01:02:49PM +0100, Jan Beulich wrote:
> >>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> > From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> 
> As long as this patch originally cam from Mukesh, From: should
> reflect that imo. Once you made changes, you S-o-b would be
> required alongside his.

And once the changes made, From will not be Mukesh email anymore?
> 
> > --- a/xen/arch/x86/hvm/svm/vmcb.c
> > +++ b/xen/arch/x86/hvm/svm/vmcb.c
> > @@ -162,7 +162,12 @@ static int construct_vmcb(struct vcpu *v)
> >      vmcb->ds.attr.bytes = 0xc93;
> >      vmcb->fs.attr.bytes = 0xc93;
> >      vmcb->gs.attr.bytes = 0xc93;
> > -    vmcb->cs.attr.bytes = 0xc9b; /* exec/read, accessed */
> > +
> > +    if ( is_pvh_vcpu(v) )
> > +        /* CS.L == 1, exec, read/write, accessed. PVH 32bitfixme. */
> > +        vmcb->cs.attr.bytes = 0xa9b;
> > +    else
> > +        vmcb->cs.attr.bytes = 0xc9b; /* exec/read, accessed */
> 
> With 32-bit support now actively being worked on, I don't think
> we want to see any new 32bitfixme-s proposed to go in. Plus it
> needs settling on whether the boot mode is to change for PVH.
> 
> Jan
> 

Re: [PATCH 1/6] pvh: domu construct vmcb 64 bit mode start

[Elena Ufimtseva]

On Tue, Jun 23, 2015 at 01:02:49PM +0100, Jan Beulich wrote:
> >>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> > From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> 
> As long as this patch originally cam from Mukesh, From: should
> reflect that imo. Once you made changes, you S-o-b would be
> required alongside his.
> 
> > --- a/xen/arch/x86/hvm/svm/vmcb.c
> > +++ b/xen/arch/x86/hvm/svm/vmcb.c
> > @@ -162,7 +162,12 @@ static int construct_vmcb(struct vcpu *v)
> >      vmcb->ds.attr.bytes = 0xc93;
> >      vmcb->fs.attr.bytes = 0xc93;
> >      vmcb->gs.attr.bytes = 0xc93;
> > -    vmcb->cs.attr.bytes = 0xc9b; /* exec/read, accessed */
> > +
> > +    if ( is_pvh_vcpu(v) )
> > +        /* CS.L == 1, exec, read/write, accessed. PVH 32bitfixme. */
> > +        vmcb->cs.attr.bytes = 0xa9b;
> > +    else
> > +        vmcb->cs.attr.bytes = 0xc9b; /* exec/read, accessed */
> 
> With 32-bit support now actively being worked on, I don't think
> we want to see any new 32bitfixme-s proposed to go in. Plus it
> needs settling on whether the boot mode is to change for PVH.

Yep, I will work on this with Boris patches in mind.

> 
> Jan
> 

Re: [PATCH 6/6] AMD-PVH: enable pvh if requirements met

[Elena Ufimtseva]

On Wed, Jun 24, 2015 at 07:24:18PM +0100, Andrew Cooper wrote:
> On 24/06/15 08:49, Jan Beulich wrote:
> >>>> On 24.06.15 at 04:34, <boris.ostrovsky@oracle.com> wrote:
> >> On 06/23/2015 08:30 AM, Jan Beulich wrote:
> >>>>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> >>>> --- a/xen/arch/x86/hvm/svm/svm.c
> >>>> +++ b/xen/arch/x86/hvm/svm/svm.c
> >>>> @@ -1444,6 +1444,9 @@ const struct hvm_function_table * __init
> >>>> start_svm(void)
> >>>>       svm_function_table.hap_capabilities = HVM_HAP_SUPERPAGE_2MB |
> >>>>           ((cpuid_edx(0x80000001) & 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0);
> >>>>   
> >>>> +    if ( cpu_has_svm_npt  && cpu_has_svm_decode )
> >>>> +        svm_function_table.pvh_supported = 1;
> >>> If svm_decode indeed is a prereq, then the earlier patch dealing
> >>> with the handle_mmio() invocations doesn't need to fiddle with
> >>> VMEXIT_INVLPG other than to maybe add a documenting ASSERT().
> >>>
> >> I am not sure we should require decode feature to be required for PVH 
> >> support. I can't remember exactly but I think this feature was first 
> >> introduced in family 15h so requiring it will leave at least family 10h 
> >> processors as not supporting PVH.
> > The question was why the dependency was added in the first place.
> > Indeed only fam 12, 15, and 16 have the field documented. Otoh
> > PVH isn't being supported universally on all VMX variants either...
> 
> Right, but this is a bug (feature?) of the current implementation and
> need fixing.
> 
> There are no technical reasons to prevent PVH guests running in any case
> where an HVM guest currently runs.
> 
> The only technical restriction I can think of is that a PVH hardware
> domain needs IOMMU support, but that is it.
> 

CCing Mukesh, maybe he will reply to as why that restriction is here.

> ~Andrew

Re: [PATCH 6/6] AMD-PVH: enable pvh if requirements met

[Mukesh Rathor]

On Wed, 24 Jun 2015 16:26:44 -0400
Elena Ufimtseva <elena.ufimtseva@oracle.com> wrote:

> On Wed, Jun 24, 2015 at 07:24:18PM +0100, Andrew Cooper wrote:
> > On 24/06/15 08:49, Jan Beulich wrote:
> > >>>> On 24.06.15 at 04:34, <boris.ostrovsky@oracle.com> wrote:
> > >> On 06/23/2015 08:30 AM, Jan Beulich wrote:
> > >>>>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> > >>>> --- a/xen/arch/x86/hvm/svm/svm.c
> > >>>> +++ b/xen/arch/x86/hvm/svm/svm.c
> > >>>> @@ -1444,6 +1444,9 @@ const struct hvm_function_table * __init
> > >>>> start_svm(void)
> > >>>>       svm_function_table.hap_capabilities =
> > >>>> HVM_HAP_SUPERPAGE_2MB | ((cpuid_edx(0x80000001) &
> > >>>> 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0); 
> > >>>> +    if ( cpu_has_svm_npt  && cpu_has_svm_decode )
> > >>>> +        svm_function_table.pvh_supported = 1;
> > >>> If svm_decode indeed is a prereq, then the earlier patch dealing
> > >>> with the handle_mmio() invocations doesn't need to fiddle with
> > >>> VMEXIT_INVLPG other than to maybe add a documenting ASSERT().
> > >>>
> > >> I am not sure we should require decode feature to be required
> > >> for PVH support. I can't remember exactly but I think this
> > >> feature was first introduced in family 15h so requiring it will
> > >> leave at least family 10h processors as not supporting PVH.
> > > The question was why the dependency was added in the first place.
> > > Indeed only fam 12, 15, and 16 have the field documented. Otoh
> > > PVH isn't being supported universally on all VMX variants
> > > either...
> > 
> > Right, but this is a bug (feature?) of the current implementation
> > and need fixing.
> > 
> > There are no technical reasons to prevent PVH guests running in any
> > case where an HVM guest currently runs.
> > 
> > The only technical restriction I can think of is that a PVH hardware
> > domain needs IOMMU support, but that is it.
> > 
> 
> CCing Mukesh, maybe he will reply to as why that restriction is here.

Hi Elena,

Basically, the restriction was to allow AMD to come on par with intel and
get phase I working on it. Then, I could just focus on handle_mmio for
INS/OUTS for both intel and amd, and if supporting !svm_decode family
of CPUs was important, then extend handle_mmio further...  

http://xen-devel.narkive.com/liQjEoV2/rfh-amd-cr-intercept-for-lmsw-clts

[In the absence of svm_decode, "mov cr" would need to go thru handle_mmio..]

thanks,
Mukesh

Re: [PATCH 1/6] pvh: domu construct vmcb 64 bit mode start

[Jan Beulich]

>>> On 24.06.15 at 22:15, <elena.ufimtseva@oracle.com> wrote:
> On Tue, Jun 23, 2015 at 01:02:49PM +0100, Jan Beulich wrote:
>> >>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
>> > From: Elena Ufimtseva <elena.ufimtseva@oracle.com>
>> > 
>> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
>> 
>> As long as this patch originally cam from Mukesh, From: should
>> reflect that imo. Once you made changes, you S-o-b would be
>> required alongside his.
> 
> And once the changes made, From will not be Mukesh email anymore?

It becomes a little fuzzy then afaiui, but personally - unless the
changes made result in a vastly different patch - I'd keep the
original From:.

Jan

Re: [PATCH 6/6] AMD-PVH: enable pvh if requirements met

[Elena Ufimtseva]

On Wed, Jun 24, 2015 at 02:41:54PM -0700, Mukesh Rathor wrote:
> On Wed, 24 Jun 2015 16:26:44 -0400
> Elena Ufimtseva <elena.ufimtseva@oracle.com> wrote:
> 
> > On Wed, Jun 24, 2015 at 07:24:18PM +0100, Andrew Cooper wrote:
> > > On 24/06/15 08:49, Jan Beulich wrote:
> > > >>>> On 24.06.15 at 04:34, <boris.ostrovsky@oracle.com> wrote:
> > > >> On 06/23/2015 08:30 AM, Jan Beulich wrote:
> > > >>>>>> On 22.06.15 at 18:37, <elena.ufimtseva@oracle.com> wrote:
> > > >>>> --- a/xen/arch/x86/hvm/svm/svm.c
> > > >>>> +++ b/xen/arch/x86/hvm/svm/svm.c
> > > >>>> @@ -1444,6 +1444,9 @@ const struct hvm_function_table * __init
> > > >>>> start_svm(void)
> > > >>>>       svm_function_table.hap_capabilities =
> > > >>>> HVM_HAP_SUPERPAGE_2MB | ((cpuid_edx(0x80000001) &
> > > >>>> 0x04000000) ? HVM_HAP_SUPERPAGE_1GB : 0); 
> > > >>>> +    if ( cpu_has_svm_npt  && cpu_has_svm_decode )
> > > >>>> +        svm_function_table.pvh_supported = 1;
> > > >>> If svm_decode indeed is a prereq, then the earlier patch dealing
> > > >>> with the handle_mmio() invocations doesn't need to fiddle with
> > > >>> VMEXIT_INVLPG other than to maybe add a documenting ASSERT().
> > > >>>
> > > >> I am not sure we should require decode feature to be required
> > > >> for PVH support. I can't remember exactly but I think this
> > > >> feature was first introduced in family 15h so requiring it will
> > > >> leave at least family 10h processors as not supporting PVH.
> > > > The question was why the dependency was added in the first place.
> > > > Indeed only fam 12, 15, and 16 have the field documented. Otoh
> > > > PVH isn't being supported universally on all VMX variants
> > > > either...
> > > 
> > > Right, but this is a bug (feature?) of the current implementation
> > > and need fixing.
> > > 
> > > There are no technical reasons to prevent PVH guests running in any
> > > case where an HVM guest currently runs.
> > > 
> > > The only technical restriction I can think of is that a PVH hardware
> > > domain needs IOMMU support, but that is it.
> > > 
> > 
> > CCing Mukesh, maybe he will reply to as why that restriction is here.
> 
> Hi Elena,
> 
> Basically, the restriction was to allow AMD to come on par with intel and
> get phase I working on it. Then, I could just focus on handle_mmio for
> INS/OUTS for both intel and amd, and if supporting !svm_decode family
> of CPUs was important, then extend handle_mmio further...  
> 
> http://xen-devel.narkive.com/liQjEoV2/rfh-amd-cr-intercept-for-lmsw-clts
> 
> [In the absence of svm_decode, "mov cr" would need to go thru handle_mmio..]

Thanks Mukesh! 

> 
> thanks,
> Mukesh
> 
> 
>