[V0 PATCH 0/6] AMD-PVH: xen domU support

[V0 PATCH 0/6] AMD-PVH: xen domU support

[Mukesh Rathor]

Hi,

Here's my first shot at AMD support of PVH domU. These
patches can also be found in git tree:

http://oss.oracle.com/git/mrathor/xen.git amd-pvh-domu-0

Please lmk of your thoughts. There's one small patch needed on linux
side to make it boot in PVH mode on AMD, that I'll have out on Monday.

Thanks,
Mukesh

[V0 PATCH 1/6] AMD-PVH: construct vmcb changes

[Mukesh Rathor]

PVH guest starts in Long 64bit paging mode. This patch modifies
construct_vmcb for that.

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

diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
index 21292bb..5df5f36 100644
--- a/xen/arch/x86/hvm/svm/vmcb.c
+++ b/xen/arch/x86/hvm/svm/vmcb.c
@@ -138,6 +138,8 @@ static int construct_vmcb(struct vcpu *v)
 
     /* Guest EFER. */
     v->arch.hvm_vcpu.guest_efer = 0;
+    if ( is_pvh_vcpu(v) )
+        v->arch.hvm_vcpu.guest_efer |= EFER_LMA;   /* PVH 32bitfixme */
     hvm_update_guest_efer(v);
 
     /* Guest segment limits. */
@@ -162,7 +164,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 +191,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.8.3.1

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

[Mukesh Rathor]

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 | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 71b8a6a..4ff4a96 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1517,18 +1517,22 @@ static void svm_vmexit_do_cpuid(struct cpu_user_regs *regs)
     if ( (inst_len = __get_instruction_length(current, INSTR_CPUID)) == 0 )
         return;
 
-    eax = regs->eax;
-    ebx = regs->ebx;
-    ecx = regs->ecx;
-    edx = regs->edx;
-
-    svm_cpuid_intercept(&eax, &ebx, &ecx, &edx);
+    if ( is_pvh_vcpu(current) )
+        pv_cpuid(regs);
+    else
+    {
+        eax = regs->eax;
+        ebx = regs->ebx;
+        ecx = regs->ecx;
+        edx = regs->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.8.3.1

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

[Mukesh Rathor]

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 | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 4ff4a96..dac16f4 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2209,6 +2209,18 @@ 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 )
+        hvm_inject_hw_exception(TRAP_gp_fault, 0);
+}
+
 void svm_vmexit_handler(struct cpu_user_regs *regs)
 {
     uint64_t exit_reason;
@@ -2470,16 +2482,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:
-- 
1.8.3.1

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

[Mukesh Rathor]

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 dac16f4..4bb4ff2 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1052,7 +1052,7 @@ static void noreturn svm_do_resume(struct vcpu *v)
         hvm_asid_flush_vcpu(v);
     }
 
-    if ( !vcpu_guestmode )
+    if ( !vcpu_guestmode && vcpu_vlapic(v)->regs )
     {
         vintr_t intr;
 
@@ -2247,7 +2247,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 && vcpu_vlapic(v)->regs ) {
         intr = vmcb_get_vintr(vmcb);
         vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI,
                    ((intr.fields.tpr & 0x0F) << 4) |
@@ -2628,15 +2628,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 && vcpu_vlapic(v)->regs )
+    {
+        /*
+         * 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.8.3.1

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

[Mukesh Rathor]

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 bd89219..7512aa4 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -1908,6 +1908,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.8.3.1

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

[Mukesh Rathor]

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 4bb4ff2..8b27a76 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1390,6 +1390,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.8.3.1

Re: [V0 PATCH 1/6] AMD-PVH: construct vmcb changes

[Roger Pau Monné]

On 16/08/14 03:53, Mukesh Rathor wrote:
> PVH guest starts in Long 64bit paging mode. This patch modifies
> construct_vmcb for that.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  xen/arch/x86/hvm/svm/vmcb.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/vmcb.c b/xen/arch/x86/hvm/svm/vmcb.c
> index 21292bb..5df5f36 100644
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -138,6 +138,8 @@ static int construct_vmcb(struct vcpu *v)
>  
>      /* Guest EFER. */
>      v->arch.hvm_vcpu.guest_efer = 0;
> +    if ( is_pvh_vcpu(v) )
> +        v->arch.hvm_vcpu.guest_efer |= EFER_LMA;   /* PVH 32bitfixme */
>      hvm_update_guest_efer(v);
>  
>      /* Guest segment limits. */
> @@ -162,7 +164,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 +191,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


I know this is already done on Intel in order to boot PVH guests, but
now that we know what we need to modify in both Intel and AMD HVM code,
do you think it would we suitable to add another parameter to
vcpu_initialise in order to tell it to setup the vcpu to boot into long
(or protected if we support 32bit PVH guests) mode (and prevent adding
more is_pvh_vcpu)?

Roger.

Re: [V0 PATCH 1/6] AMD-PVH: construct vmcb changes

[Mukesh Rathor]

On Mon, 18 Aug 2014 11:10:54 +0200
Roger Pau Monné <roger.pau@citrix.com> wrote:

> On 16/08/14 03:53, Mukesh Rathor wrote:
> > PVH guest starts in Long 64bit paging mode. This patch modifies
> > construct_vmcb for that.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > ---
> >  xen/arch/x86/hvm/svm/vmcb.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/svm/vmcb.c
> > b/xen/arch/x86/hvm/svm/vmcb.c index 21292bb..5df5f36 100644
> > --- a/xen/arch/x86/hvm/svm/vmcb.c
> > +++ b/xen/arch/x86/hvm/svm/vmcb.c
> > @@ -138,6 +138,8 @@ static int construct_vmcb(struct vcpu *v)
> >  
> >      /* Guest EFER. */
> >      v->arch.hvm_vcpu.guest_efer = 0;
> > +    if ( is_pvh_vcpu(v) )
> > +        v->arch.hvm_vcpu.guest_efer |= EFER_LMA;   /* PVH
> > 32bitfixme */ hvm_update_guest_efer(v);
....
> 
> 
> I know this is already done on Intel in order to boot PVH guests, but
> now that we know what we need to modify in both Intel and AMD HVM
> code, do you think it would we suitable to add another parameter to
> vcpu_initialise in order to tell it to setup the vcpu to boot into
> long (or protected if we support 32bit PVH guests) mode (and prevent
> adding more is_pvh_vcpu)?

Well, we can defer adding parameter to vcpu_initialise to when 32bit
work gets done. For now, we could move common hvm fields initialization 
to hvm_vcpu_initialise.. that way, we'd have fewer is_pvh. I can
submit a patch if Jan is ok with that.

Mukesh


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

Re: [V0 PATCH 1/6] AMD-PVH: construct vmcb changes

[Boris Ostrovsky]

On 08/18/2014 07:46 PM, Mukesh Rathor wrote:
> On Mon, 18 Aug 2014 11:10:54 +0200
> Roger Pau Monné <roger.pau@citrix.com> wrote:
>
>> On 16/08/14 03:53, Mukesh Rathor wrote:
>>> PVH guest starts in Long 64bit paging mode. This patch modifies
>>> construct_vmcb for that.
>>>
>>> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
>>> ---
>>>   xen/arch/x86/hvm/svm/vmcb.c | 18 +++++++++++++++---
>>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/svm/vmcb.c
>>> b/xen/arch/x86/hvm/svm/vmcb.c index 21292bb..5df5f36 100644
>>> --- a/xen/arch/x86/hvm/svm/vmcb.c
>>> +++ b/xen/arch/x86/hvm/svm/vmcb.c
>>> @@ -138,6 +138,8 @@ static int construct_vmcb(struct vcpu *v)
>>>   
>>>       /* Guest EFER. */
>>>       v->arch.hvm_vcpu.guest_efer = 0;
>>> +    if ( is_pvh_vcpu(v) )
>>> +        v->arch.hvm_vcpu.guest_efer |= EFER_LMA;   /* PVH
>>> 32bitfixme */ hvm_update_guest_efer(v);
> ....
>>
>> I know this is already done on Intel in order to boot PVH guests, but
>> now that we know what we need to modify in both Intel and AMD HVM
>> code, do you think it would we suitable to add another parameter to
>> vcpu_initialise in order to tell it to setup the vcpu to boot into
>> long (or protected if we support 32bit PVH guests) mode (and prevent
>> adding more is_pvh_vcpu)?
> Well, we can defer adding parameter to vcpu_initialise to when 32bit
> work gets done. For now, we could move common hvm fields initialization
> to hvm_vcpu_initialise.. that way, we'd have fewer is_pvh. I can
> submit a patch if Jan is ok with that.

Setting LMA bit for PVH guest is in fact already done by 
hvm_vcpu_initialise()  so the change above is unnecessary.

-boris


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

Re: [V0 PATCH 1/6] AMD-PVH: construct vmcb changes

[Mukesh Rathor]

On Mon, 18 Aug 2014 20:59:00 -0400
Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:

> On 08/18/2014 07:46 PM, Mukesh Rathor wrote:
> > On Mon, 18 Aug 2014 11:10:54 +0200
> > Roger Pau Monné <roger.pau@citrix.com> wrote:
> >
> >> On 16/08/14 03:53, Mukesh Rathor wrote:
> >>> PVH guest starts in Long 64bit paging mode. This patch modifies
> >>> construct_vmcb for that.
> >>>
> >>> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> >>> ---
> >>>   xen/arch/x86/hvm/svm/vmcb.c | 18 +++++++++++++++---
> >>>   1 file changed, 15 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/arch/x86/hvm/svm/vmcb.c
> >>> b/xen/arch/x86/hvm/svm/vmcb.c index 21292bb..5df5f36 100644
> >>> --- a/xen/arch/x86/hvm/svm/vmcb.c
> >>> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> >>> @@ -138,6 +138,8 @@ static int construct_vmcb(struct vcpu *v)
> >>>   
> >>>       /* Guest EFER. */
> >>>       v->arch.hvm_vcpu.guest_efer = 0;
> >>> +    if ( is_pvh_vcpu(v) )
> >>> +        v->arch.hvm_vcpu.guest_efer |= EFER_LMA;   /* PVH
> >>> 32bitfixme */ hvm_update_guest_efer(v);
> > ....
> >>
> >> I know this is already done on Intel in order to boot PVH guests,
> >> but now that we know what we need to modify in both Intel and AMD
> >> HVM code, do you think it would we suitable to add another
> >> parameter to vcpu_initialise in order to tell it to setup the vcpu
> >> to boot into long (or protected if we support 32bit PVH guests)
> >> mode (and prevent adding more is_pvh_vcpu)?
> > Well, we can defer adding parameter to vcpu_initialise to when 32bit
> > work gets done. For now, we could move common hvm fields
> > initialization to hvm_vcpu_initialise.. that way, we'd have fewer
> > is_pvh. I can submit a patch if Jan is ok with that.
> 
> Setting LMA bit for PVH guest is in fact already done by 
> hvm_vcpu_initialise()  so the change above is unnecessary.

yeah, that one we can leave out, but there still remains 

+    if ( is_pvh_vcpu(v) )
+        v->arch.hvm_vcpu.guest_cr[0] |= X86_CR0_PG;

and

+    v->arch.hvm_vcpu.guest_cr[4] = is_pvh_vcpu(v) ? X86_CR4_PAE : 0;

that could be moved to hvm_vcpu_initialise()

-Mukesh



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

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

[Boris Ostrovsky]

On 08/15/2014 09:53 PM, Mukesh Rathor wrote:
> 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 dac16f4..4bb4ff2 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1052,7 +1052,7 @@ static void noreturn svm_do_resume(struct vcpu *v)
>           hvm_asid_flush_vcpu(v);
>       }
>   
> -    if ( !vcpu_guestmode )
> +    if ( !vcpu_guestmode && vcpu_vlapic(v)->regs )

I think checking explicitly for PVH (i.e. is_pvh_domain()) would be 
better. Here and below, obviously.

-boris

>       {
>           vintr_t intr;
>   
> @@ -2247,7 +2247,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 && vcpu_vlapic(v)->regs ) {
>           intr = vmcb_get_vintr(vmcb);
>           vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI,
>                      ((intr.fields.tpr & 0x0F) << 4) |
> @@ -2628,15 +2628,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 && vcpu_vlapic(v)->regs )
> +    {
> +        /*
> +         * 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)

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

[Boris Ostrovsky]

On 08/15/2014 09:53 PM, Mukesh Rathor wrote:
> 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 bd89219..7512aa4 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -1908,6 +1908,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:

I suspect that TSC_MODE DEFAULT may actually work for PVH: since we 
don't support migration it should be equivalent to NEVER_EMULATE.

If you replace in tsc_set_info() is_hvm_domain() with 
is_hvm_container_domain() and allow TSC_MODE_DEFAULT along with 
TSC_MODE_NEVER_EMULATE under 'if (is_pvh_domain())' then it seems to me 
that things should be fine.


-boris

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

[Boris Ostrovsky]

On 08/18/2014 09:38 PM, Boris Ostrovsky wrote:
> On 08/15/2014 09:53 PM, Mukesh Rathor wrote:
>> 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 bd89219..7512aa4 100644
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -1908,6 +1908,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:
>
> I suspect that TSC_MODE DEFAULT may actually work for PVH: since we 
> don't support migration it should be equivalent to NEVER_EMULATE.
>
> If you replace in tsc_set_info() is_hvm_domain() with 
> is_hvm_container_domain() and allow TSC_MODE_DEFAULT along with 
> TSC_MODE_NEVER_EMULATE under 'if (is_pvh_domain())' then it seems to 
> me that things should be fine.

(hit RET too soon)

... and if TSC_MODE DEFAULT does work then you don't need to set khz 
above since it (as well as some other things) will be set from 
arch_domain_create().

-boris

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

[Mukesh Rathor]

On Mon, 18 Aug 2014 21:38:35 -0400
Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:

> On 08/15/2014 09:53 PM, Mukesh Rathor wrote:
> > 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 bd89219..7512aa4 100644
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -1908,6 +1908,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:
> 
> I suspect that TSC_MODE DEFAULT may actually work for PVH: since we 
> don't support migration it should be equivalent to NEVER_EMULATE.
> 
> If you replace in tsc_set_info() is_hvm_domain() with 
> is_hvm_container_domain() and allow TSC_MODE_DEFAULT along with 
> TSC_MODE_NEVER_EMULATE under 'if (is_pvh_domain())' then it seems to
> me that things should be fine.

Nope, already tried that. Something is not being set properly in terms
of scaling or offset, and the first timer gets set years in future! Since
you are already going to look into it, I'll let you :)..

thanks,
Mukesh

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

[Jan Beulich]

>>> Boris Ostrovsky <boris.ostrovsky@oracle.com> 08/19/14 3:17 AM >>>
>On 08/15/2014 09:53 PM, Mukesh Rathor wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -1052,7 +1052,7 @@ static void noreturn svm_do_resume(struct vcpu *v)
>>           hvm_asid_flush_vcpu(v);
>>       }
>>   
>> -    if ( !vcpu_guestmode )
>> +    if ( !vcpu_guestmode && vcpu_vlapic(v)->regs )
>
>I think checking explicitly for PVH (i.e. is_pvh_domain()) would be 
>better. Here and below, obviously.

Yeah, I guess so for now, as it'll make it easier to locate the place when
ultimately changing to more fine grained checks.

Jan

Re: [V0 PATCH 1/6] AMD-PVH: construct vmcb changes

[Jan Beulich]

>>> On 16.08.14 at 03:53, <mukesh.rathor@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/svm/vmcb.c
> +++ b/xen/arch/x86/hvm/svm/vmcb.c
> @@ -138,6 +138,8 @@ static int construct_vmcb(struct vcpu *v)
>  
>      /* Guest EFER. */
>      v->arch.hvm_vcpu.guest_efer = 0;
> +    if ( is_pvh_vcpu(v) )
> +        v->arch.hvm_vcpu.guest_efer |= EFER_LMA;   /* PVH 32bitfixme */

Style-wise, just like you do below, I'd prefer this to be an if/else or
(here and below) using ?:. But I'll leave the final say to the SVM
maintainers...

Jan

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

[Jan Beulich]

>>> On 16.08.14 at 03:53, <mukesh.rathor@oracle.com> wrote:
> 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.

Is that really correct without slight adjustments to the function? I'm
particularly worried about (some of) the uses of vio there...

> handle_mmio call is forbidden for pvh.
> 
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
>  xen/arch/x86/hvm/svm/svm.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> index 4ff4a96..dac16f4 100644
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -2209,6 +2209,18 @@ 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 )
> +        hvm_inject_hw_exception(TRAP_gp_fault, 0);

I think this is too simplistic - when ctxt.exn_pending is set, you
ought to be delivering _that_ exception instead of #GP.

> @@ -2470,16 +2482,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:

There's another use of handle_mmio() right below here - shouldn't
that get replaced too (even if not strictly required by PVH)?

Also - how come at least the use of the function in VMX's
EXIT_REASON_IO_INSTRUCTION handling is no problem for PVH,
but SVM's VMEXIT_IOIO one is?

Jan

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

[Jan Beulich]

>>> On 16.08.14 at 03:53, <mukesh.rathor@oracle.com> wrote:
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1052,7 +1052,7 @@ static void noreturn svm_do_resume(struct vcpu *v)
>          hvm_asid_flush_vcpu(v);
>      }
>  
> -    if ( !vcpu_guestmode )
> +    if ( !vcpu_guestmode && vcpu_vlapic(v)->regs )

I may have said this in reply to another of your patches already, but
in the case I don't recall correctly: Please make this !is_pvh_vcpu() or
introduce a proper is_...() accessor so whoever is going to do the
future conversion to a more fine grained model will have a way to
actually spot these.

Jan

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

[Mukesh Rathor]

On Fri, 22 Aug 2014 10:50:01 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 16.08.14 at 03:53, <mukesh.rathor@oracle.com> wrote:
> > 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.
> 
> Is that really correct without slight adjustments to the function? I'm
> particularly worried about (some of) the uses of vio there...
> 
> > handle_mmio call is forbidden for pvh.
> > 
> > Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> > ---
> >  xen/arch/x86/hvm/svm/svm.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
> > index 4ff4a96..dac16f4 100644
> > --- a/xen/arch/x86/hvm/svm/svm.c
> > +++ b/xen/arch/x86/hvm/svm/svm.c
> > @@ -2209,6 +2209,18 @@ 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 )
> > +        hvm_inject_hw_exception(TRAP_gp_fault, 0);
> 
> I think this is too simplistic - when ctxt.exn_pending is set, you
> ought to be delivering _that_ exception instead of #GP.

Right, the existing code just injects GP, which is not quite ideal. 

> > @@ -2470,16 +2482,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:
> 
> There's another use of handle_mmio() right below here - shouldn't
> that get replaced too (even if not strictly required by PVH)?
> 
> Also - how come at least the use of the function in VMX's
> EXIT_REASON_IO_INSTRUCTION handling is no problem for PVH,
> but SVM's VMEXIT_IOIO one is?

Yup, missed that one. That would need to be addressed. 

I guess the first step would be to do a non-pvh patch to fix calling
handle_mmio for non-mmio purposes.  Since, it applies to both vmx/svm, 
perhaps an hvm function. Let me do that first, and then pvh can
piggyback on that.

thanks
Mukesh

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

[Jan Beulich]

>>> On 22.08.14 at 20:52, <mukesh.rathor@oracle.com> wrote:
> On Fri, 22 Aug 2014 10:50:01 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
>> Also - how come at least the use of the function in VMX's
>> EXIT_REASON_IO_INSTRUCTION handling is no problem for PVH,
>> but SVM's VMEXIT_IOIO one is?
> 
> Yup, missed that one. That would need to be addressed. 
> 
> I guess the first step would be to do a non-pvh patch to fix calling
> handle_mmio for non-mmio purposes.  Since, it applies to both vmx/svm, 
> perhaps an hvm function. Let me do that first, and then pvh can
> piggyback on that.

Problem being that INS and OUTS can very well address MMIO
on the memory side of the operation (while right now
hvmemul_rep_{ins,outs}() fail such operations, this merely means
they'd get emulated one by one instead of accelerated as multiple
ops in one go).

Also looking at handle_mmio() once again - it being just a relatively
thin wrapper around hvm_emulate_one(), can you remind me again
what in this small function it was that breaks on PVH? It would seem
to me that you'd rather want a clone of hvm_emulate_one() with a
different struct x86_emulate_ops passed in.

Jan

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

[Mukesh Rathor]

On Mon, 25 Aug 2014 08:10:38 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 22.08.14 at 20:52, <mukesh.rathor@oracle.com> wrote:
> > On Fri, 22 Aug 2014 10:50:01 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> >> Also - how come at least the use of the function in VMX's
> >> EXIT_REASON_IO_INSTRUCTION handling is no problem for PVH,
> >> but SVM's VMEXIT_IOIO one is?
> > 
> > Yup, missed that one. That would need to be addressed. 
> > 
> > I guess the first step would be to do a non-pvh patch to fix calling
> > handle_mmio for non-mmio purposes.  Since, it applies to both
> > vmx/svm, perhaps an hvm function. Let me do that first, and then
> > pvh can piggyback on that.
> 
> Problem being that INS and OUTS can very well address MMIO
> on the memory side of the operation (while right now
> hvmemul_rep_{ins,outs}() fail such operations, this merely means
> they'd get emulated one by one instead of accelerated as multiple
> ops in one go).
> 
> Also looking at handle_mmio() once again - it being just a relatively
> thin wrapper around hvm_emulate_one(), can you remind me again
> what in this small function it was that breaks on PVH? It would seem

handle_mmio -> hvmemul_do_io -> hvm_mmio_intercept(), last one is
fatal as not all handlers are safe for pvh. We had discussed creating
pvh_mmio_handlers[] parallel to hvm_mmio_handlers[], which for now
would be empty, but can be expanded in future. Then hvm_mmio_intercept()
will chose one or the other depending on is_pvh. Or, I could just skip 
hvm_mmio_handlers if pvh in hvm_mmio_intercept(), and pvh_mmio_handlers[]
can be added in future. Either way.

With above change, we can remove !pvh assert from handle_mmio, 
and leave svm/vmx code unchanged (so it will keep calling handle_mmio).
This would greatly help amd domU patches since time seems to be running
short.

thanks
mukesh

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

[Jan Beulich]

>>> On 26.08.14 at 03:53, <mukesh.rathor@oracle.com> wrote:
> On Mon, 25 Aug 2014 08:10:38 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> >>> On 22.08.14 at 20:52, <mukesh.rathor@oracle.com> wrote:
>> > On Fri, 22 Aug 2014 10:50:01 +0100
>> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> >> Also - how come at least the use of the function in VMX's
>> >> EXIT_REASON_IO_INSTRUCTION handling is no problem for PVH,
>> >> but SVM's VMEXIT_IOIO one is?
>> > 
>> > Yup, missed that one. That would need to be addressed. 
>> > 
>> > I guess the first step would be to do a non-pvh patch to fix calling
>> > handle_mmio for non-mmio purposes.  Since, it applies to both
>> > vmx/svm, perhaps an hvm function. Let me do that first, and then
>> > pvh can piggyback on that.
>> 
>> Problem being that INS and OUTS can very well address MMIO
>> on the memory side of the operation (while right now
>> hvmemul_rep_{ins,outs}() fail such operations, this merely means
>> they'd get emulated one by one instead of accelerated as multiple
>> ops in one go).
>> 
>> Also looking at handle_mmio() once again - it being just a relatively
>> thin wrapper around hvm_emulate_one(), can you remind me again
>> what in this small function it was that breaks on PVH? It would seem
> 
> handle_mmio -> hvmemul_do_io -> hvm_mmio_intercept(), last one is
> fatal as not all handlers are safe for pvh.

But you see - that is exactly my point: Avoiding handle_mmio() alone
doesn't buy you anything, as hvmemul_do_io() isn't being called
directly from that function, but indirectly via the actors specified in
hvm_emulate_ops. Which gets me back to suggesting that you need
a different struct x86_emulate_ops instance to deal with these non-
MMIO emulation needs.

Jan

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

[Mukesh Rathor]

On Tue, 26 Aug 2014 08:17:06 +0100
"Jan Beulich" <JBeulich@suse.com> wrote:

> >>> On 26.08.14 at 03:53, <mukesh.rathor@oracle.com> wrote:
> > On Mon, 25 Aug 2014 08:10:38 +0100
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >> >>> On 22.08.14 at 20:52, <mukesh.rathor@oracle.com> wrote:
> >> > On Fri, 22 Aug 2014 10:50:01 +0100
> >> > "Jan Beulich" <JBeulich@suse.com> wrote:
> >> >> Also - how come at least the use of the function in VMX's
> >> >> EXIT_REASON_IO_INSTRUCTION handling is no problem for PVH,
> >> >> but SVM's VMEXIT_IOIO one is?
> >> > 
> >> > Yup, missed that one. That would need to be addressed. 
> >> > 
> >> > I guess the first step would be to do a non-pvh patch to fix
> >> > calling handle_mmio for non-mmio purposes.  Since, it applies to
> >> > both vmx/svm, perhaps an hvm function. Let me do that first, and
> >> > then pvh can piggyback on that.
> >> 
> >> Problem being that INS and OUTS can very well address MMIO
> >> on the memory side of the operation (while right now
> >> hvmemul_rep_{ins,outs}() fail such operations, this merely means
> >> they'd get emulated one by one instead of accelerated as multiple
> >> ops in one go).
> >> 
> >> Also looking at handle_mmio() once again - it being just a
> >> relatively thin wrapper around hvm_emulate_one(), can you remind
> >> me again what in this small function it was that breaks on PVH? It
> >> would seem
> > 
> > handle_mmio -> hvmemul_do_io -> hvm_mmio_intercept(), last one is
> > fatal as not all handlers are safe for pvh.
> 
> But you see - that is exactly my point: Avoiding handle_mmio() alone
> doesn't buy you anything, as hvmemul_do_io() isn't being called
> directly from that function, but indirectly via the actors specified
> in hvm_emulate_ops. Which gets me back to suggesting that you need
> a different struct x86_emulate_ops instance to deal with these non-
> MMIO emulation needs.

Hmm... going that route seems to duplicate lot of code in 
hvm_emulate_one, but I'm not suggesting that's a roadblock.

FWIW, looking at the code again, the only unsafe call to handle_mmio 
for pvh that resulted in call to hvm_mmio_intercept has been taken care of:

hvm_hap_nested_page_fault():
    if ( (p2mt == p2m_mmio_dm) ||
          (access_w && (p2mt == p2m_ram_ro)) )
    {
        put_gfn(p2m->domain, gfn);
        rc = 0;

        if ( unlikely(is_pvh_vcpu(v)) )   <----
            goto out; 
        if ( !handle_mmio() )
.....

So, at present, the only call to handle_mmio for PVH should be vmx
INS/OUTS which would not go to hvm_mmio_intercept, but hvm_portio_intercept.
Which means, handle_mmio is safe for pvh now and we could even remove
the ASSERT !is_pvh_guest.

In conclusion, 3 options:
  1. Create a different struct x86_emulate_ops. 
        - Cleaner, but code very similar to hvm_emulate_one

  2. Remove !PVH ASSERT in handle_mmio now that we know the only path for PVH
     is IO instructions. However, conceptually there's no mmio emul for pvh so
     nice to make that statement with the ASSERT.

  3. Replace call to handle_mmio in EXIT_REASON_IO_INSTRUCTION intercept with 
     hvm_emulate_one. This path will never result in call to 
     hvm_mmio_intercept(), hence safe to do.

If you still wanna go with #1, I'll have to punt it to Boris when he's
back, and we should change AMD support in 4.5 to maybe, and also realise vmx
INS/OUTS will remain broken on debug builds because of the ASSERT in 
handle_mmio.  Otherwise, I can whip out new version of patches, my preference 
would be #3.  I could create a new function say hvm_emulate_io_intercept which
will call hvm_emulate_one(), so one can easily change it to do #1 in future.

thanks,
Mukesh

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

[Jan Beulich]

>>> On 28.08.14 at 03:07, <mukesh.rathor@oracle.com> wrote:
> On Tue, 26 Aug 2014 08:17:06 +0100
> "Jan Beulich" <JBeulich@suse.com> wrote:
>> >>> On 26.08.14 at 03:53, <mukesh.rathor@oracle.com> wrote:
>> > On Mon, 25 Aug 2014 08:10:38 +0100
>> > "Jan Beulich" <JBeulich@suse.com> wrote:
>> >> Also looking at handle_mmio() once again - it being just a
>> >> relatively thin wrapper around hvm_emulate_one(), can you remind
>> >> me again what in this small function it was that breaks on PVH? It
>> >> would seem
>> > 
>> > handle_mmio -> hvmemul_do_io -> hvm_mmio_intercept(), last one is
>> > fatal as not all handlers are safe for pvh.
>> 
>> But you see - that is exactly my point: Avoiding handle_mmio() alone
>> doesn't buy you anything, as hvmemul_do_io() isn't being called
>> directly from that function, but indirectly via the actors specified
>> in hvm_emulate_ops. Which gets me back to suggesting that you need
>> a different struct x86_emulate_ops instance to deal with these non-
>> MMIO emulation needs.
> 
> Hmm... going that route seems to duplicate lot of code in 
> hvm_emulate_one, but I'm not suggesting that's a roadblock.

Are you also at least casually looking at what changes other people
propose on the list? One of the recent series already introduces a
second such instance (in order to be able to discard writes), and
does so without duplicating much code.

> So, at present, the only call to handle_mmio for PVH should be vmx
> INS/OUTS which would not go to hvm_mmio_intercept, but hvm_portio_intercept.
> Which means, handle_mmio is safe for pvh now and we could even remove
> the ASSERT !is_pvh_guest.

In this argumentation you leave aside that all the code paths leading
to hvm_mmio_intercept() are only known to be dead now, but a live
code path reaching them could be introduced at any time without
anyone explicitly noticing (until it possibly gets found to be a security
issue). I'm afraid you continue to think the "just make it work" way
rather than "make it maintainable code", yet the latter is required for
getting PVH out of the experimental corner.

> In conclusion, 3 options:
>   1. Create a different struct x86_emulate_ops. 
>         - Cleaner, but code very similar to hvm_emulate_one

As per above - there likely is a way to do this without much code
duplication.

>   2. Remove !PVH ASSERT in handle_mmio now that we know the only path for PVH
>      is IO instructions. However, conceptually there's no mmio emul for pvh so
>      nice to make that statement with the ASSERT.

Indeed, removing such an ASSERT() is at least premature; I'm in no
way opposed to keeping it indefinitely.

>   3. Replace call to handle_mmio in EXIT_REASON_IO_INSTRUCTION intercept with 
>      hvm_emulate_one. This path will never result in call to 
>      hvm_mmio_intercept(), hence safe to do.

This is simply not true - apparently you're only considering the path
hvmemul_rep_{ins,outs}() -> hvmemul_do_pio(). But as said before,
if the memory side of these instructions happens to be MMIO (don't
forget that you want to support pass-through at some point), the
former of these will bail before reaching hvmemul_do_pio(), making
the emulator retry via the hvmemul_{read,write}() paths, which do
have calls to hvmemul_do_mmio(). As that namely happens when
hvm_copy_{from,to}_guest_virt() return HVMCOPY_bad_gfn_to_mfn,
I actually think that this code path is reachable even without
pass-through support (by the guest accessing some non-RAM
region). Yet again, even if - by going through all the code paths and
checking that is_pvh_...() checks prevent this from happening today
- this can't happen now, it _still_ is a latent issue possible to surface
at any time, which we ought to avoid.

Jan

Re: [V0 PATCH 0/6] AMD-PVH: xen domU support

[Roger Pau Monné]

El 16/08/14 a les 3.53, Mukesh Rathor ha escrit:
> Hi,
> 
> Here's my first shot at AMD support of PVH domU. These
> patches can also be found in git tree:
> 
> http://oss.oracle.com/git/mrathor/xen.git amd-pvh-domu-0

Is this aiming at the 4.5 release? I would really like to see PVH AMD
support in 4.5.

Roger.

Re: [V0 PATCH 0/6] AMD-PVH: xen domU support

[Mukesh Rathor]

On Tue, 30 Sep 2014 18:04:36 +0200
Roger Pau Monné <roger.pau@citrix.com> wrote:

> El 16/08/14 a les 3.53, Mukesh Rathor ha escrit:
> > Hi,
> > 
> > Here's my first shot at AMD support of PVH domU. These
> > patches can also be found in git tree:
> > 
> > http://oss.oracle.com/git/mrathor/xen.git amd-pvh-domu-0
> 
> Is this aiming at the 4.5 release? I would really like to see PVH AMD
> support in 4.5.
> 
> Roger.

Hey Roger,

I had indicated it was not looking good:

http://permalink.gmane.org/gmane.comp.emulators.xen.devel/212995

Basically, we need to straighten the handle_mmio calls in the vmx/svm
exit paths for IO emulation. However, at this point, I've moved out of
the open source group at Oracle, and Boris/Konrad have taken over. If
the handle_mmio gets straighten out, I can probably still spend some
time and submit the patches. The patches are straightforward other than
that. I was able to test them out going thru handle_mmio for io
emulation temporarily :).. (Please see patch 3/6 in this series for
more info if you wanna take a stab at it).

thanks,
Mukesh

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

Re: [V0 PATCH 0/6] AMD-PVH: xen domU support

[Mukesh Rathor]

On Tue, 30 Sep 2014 14:38:53 -0700
Mukesh Rathor <mukesh.rathor@oracle.com> wrote:

> On Tue, 30 Sep 2014 18:04:36 +0200
> Roger Pau Monné <roger.pau@citrix.com> wrote:
> 
> > El 16/08/14 a les 3.53, Mukesh Rathor ha escrit:
> > > Hi,
> > > 
> > > Here's my first shot at AMD support of PVH domU. These
> > > patches can also be found in git tree:
> > > 
> > > http://oss.oracle.com/git/mrathor/xen.git amd-pvh-domu-0
> > 
> > Is this aiming at the 4.5 release? I would really like to see PVH
> > AMD support in 4.5.
> > 
> > Roger.
> 
> Hey Roger,
> 
> I had indicated it was not looking good:
> 
> http://permalink.gmane.org/gmane.comp.emulators.xen.devel/212995
> 
> Basically, we need to straighten the handle_mmio calls in the vmx/svm
> exit paths for IO emulation. However, at this point, I've moved out of

Oh, and there is one more path where handle_mmio is
inappropriately called in vmexit, that is for lmsw/clts instructions.

thanks
Mukesh



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

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

[Mukesh Rathor]

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 dac16f4..4bb4ff2 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1052,7 +1052,7 @@ static void noreturn svm_do_resume(struct vcpu *v)
         hvm_asid_flush_vcpu(v);
     }
 
-    if ( !vcpu_guestmode )
+    if ( !vcpu_guestmode && vcpu_vlapic(v)->regs )
     {
         vintr_t intr;
 
@@ -2247,7 +2247,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 && vcpu_vlapic(v)->regs ) {
         intr = vmcb_get_vintr(vmcb);
         vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI,
                    ((intr.fields.tpr & 0x0F) << 4) |
@@ -2628,15 +2628,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 && vcpu_vlapic(v)->regs )
+    {
+        /*
+         * 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.8.3.1