[PATCH for-xen-4.5] x86/pvh/vpmu: Disable VPMU for PVH guests

[PATCH for-xen-4.5] x86/pvh/vpmu: Disable VPMU for PVH guests

[Boris Ostrovsky]

Currently when VPMU is enabled on a system both HVM and PVH VPCUs will
initialize their VPMUs, including setting up vpmu_ops. As result even
though VPMU will not work for PVH guests (APIC is not supported there),
the guest may decide to perform a write to a PMU MSR. This will cause a
call to is_vlapic_lvtpc_enabled() which will crash the hypervisor, e.g.:

 (XEN) Xen call trace:
 (XEN)    [<ffff82d0801ca06f>] is_vlapic_lvtpc_enabled+0x13/0x22
 (XEN)    [<ffff82d0801e2a15>] core2_vpmu_do_wrmsr+0x415/0x589
 (XEN)    [<ffff82d0801cedaa>] vpmu_do_wrmsr+0x2a/0x33
 (XEN)    [<ffff82d0801dd648>] vmx_msr_write_intercept+0x268/0x557
 (XEN)    [<ffff82d0801bcd2e>] hvm_msr_write_intercept+0x36c/0x39b
 (XEN)    [<ffff82d0801e0a0e>] vmx_vmexit_handler+0x1082/0x185b
 (XEN)    [<ffff82d0801e74c1>] vmx_asm_vmexit_handler+0x41/0xc0

If we prevent VPMU from being initialized on PVH guests we will avoid
those accesses.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/hvm/vpmu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/x86/hvm/vpmu.c b/xen/arch/x86/hvm/vpmu.c
index aec7b5f..4daa993 100644
--- a/xen/arch/x86/hvm/vpmu.c
+++ b/xen/arch/x86/hvm/vpmu.c
@@ -218,6 +218,9 @@ void vpmu_initialise(struct vcpu *v)
     struct vpmu_struct *vpmu = vcpu_vpmu(v);
     uint8_t vendor = current_cpu_data.x86_vendor;
 
+    if ( is_pvh_vcpu(v) )
+        return;
+
     if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
         vpmu_destroy(v);
     vpmu_clear(vpmu);
-- 
1.8.1.4

Re: [PATCH for-xen-4.5] x86/pvh/vpmu: Disable VPMU for PVH guests

[Jan Beulich]

>>> On 24.11.14 at 20:49, <boris.ostrovsky@oracle.com> wrote:
> Currently when VPMU is enabled on a system both HVM and PVH VPCUs will
> initialize their VPMUs, including setting up vpmu_ops. As result even
> though VPMU will not work for PVH guests (APIC is not supported there),
> the guest may decide to perform a write to a PMU MSR. This will cause a
> call to is_vlapic_lvtpc_enabled() which will crash the hypervisor, e.g.:
> 
>  (XEN) Xen call trace:
>  (XEN)    [<ffff82d0801ca06f>] is_vlapic_lvtpc_enabled+0x13/0x22
>  (XEN)    [<ffff82d0801e2a15>] core2_vpmu_do_wrmsr+0x415/0x589
>  (XEN)    [<ffff82d0801cedaa>] vpmu_do_wrmsr+0x2a/0x33
>  (XEN)    [<ffff82d0801dd648>] vmx_msr_write_intercept+0x268/0x557
>  (XEN)    [<ffff82d0801bcd2e>] hvm_msr_write_intercept+0x36c/0x39b
>  (XEN)    [<ffff82d0801e0a0e>] vmx_vmexit_handler+0x1082/0x185b
>  (XEN)    [<ffff82d0801e74c1>] vmx_asm_vmexit_handler+0x41/0xc0
> 
> If we prevent VPMU from being initialized on PVH guests we will avoid
> those accesses.

I think this fix is too specific; instead we should mark the LAPIC
disabled, which will implicitly prevent the issue afaict - see below.

Jan

x86/PVH: properly disable vLAPIC

Rather than guarding higher level operations (like vPMU initialization
as suggested by Boris in
http://lists.xenproject.org/archives/html/xen-devel/2014-11/msg02278.html)
mark the vLAPIC hardware disabled for PVH guests and prevent it from
getting moved out of this state.

Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -2217,8 +2217,7 @@ int hvm_vcpu_initialise(struct vcpu *v)
         goto fail1;
 
     /* NB: vlapic_init must be called before hvm_funcs.vcpu_initialise */
-    if ( is_hvm_vcpu(v) )
-        rc = vlapic_init(v);
+    rc = vlapic_init(v);
     if ( rc != 0 ) /* teardown: vlapic_destroy */
         goto fail2;
 
@@ -4483,7 +4482,8 @@ int hvm_msr_write_intercept(unsigned int
         break;
 
     case MSR_IA32_APICBASE:
-        if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
+        if ( unlikely(is_pvh_vcpu(v)) ||
+             !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
             goto gp_fault;
         break;
 
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -1429,6 +1429,12 @@ int vlapic_init(struct vcpu *v)
 
     HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "%d", v->vcpu_id);
 
+    if ( is_pvh_vcpu(v) )
+    {
+        vlapic->hw.disabled = VLAPIC_HW_DISABLED;
+        return 0;
+    }
+
     vlapic->pt.source = PTSRC_lapic;
 
     if (vlapic->regs_page == NULL)

Re: [PATCH for-xen-4.5] x86/pvh/vpmu: Disable VPMU for PVH guests

[Boris Ostrovsky]

On 11/25/2014 03:45 AM, Jan Beulich wrote:
> @@ -1429,6 +1429,12 @@ int vlapic_init(struct vcpu *v)
>   
>       HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "%d", v->vcpu_id);
>   
> +    if ( is_pvh_vcpu(v) )
> +    {
> +        vlapic->hw.disabled = VLAPIC_HW_DISABLED;


I did consider doing that but I thought that this flag is meant to be 
set when the guest clears MSR_IA32_APICBASE_ENABLE to disable APIC and 
therefore I'd be overloading it (the flag) in a way.

Regardless, do you think that disabling VPMU for PVH is worth anyway?

-boris



> +        return 0;
> +    }
> +
>       vlapic->pt.source = PTSRC_lapic;
>   
>       if (vlapic->regs_page == NULL)
>
>
>

Re: [PATCH for-xen-4.5] x86/pvh/vpmu: Disable VPMU for PVH guests

[Jan Beulich]

>>> On 25.11.14 at 15:38, <boris.ostrovsky@oracle.com> wrote:
> On 11/25/2014 03:45 AM, Jan Beulich wrote:
>> @@ -1429,6 +1429,12 @@ int vlapic_init(struct vcpu *v)
>>   
>>       HVM_DBG_LOG(DBG_LEVEL_VLAPIC, "%d", v->vcpu_id);
>>   
>> +    if ( is_pvh_vcpu(v) )
>> +    {
>> +        vlapic->hw.disabled = VLAPIC_HW_DISABLED;
> 
> 
> I did consider doing that but I thought that this flag is meant to be 
> set when the guest clears MSR_IA32_APICBASE_ENABLE to disable APIC and 
> therefore I'd be overloading it (the flag) in a way.

There's no overloading here - we're then simply treating all PVH
vCPU-s as having permanently hardware-disabled LAPICs (reflecting
current reality).

> Regardless, do you think that disabling VPMU for PVH is worth anyway?

That depends on what (bad) consequences not doing so has.

Jan

Re: [PATCH for-xen-4.5] x86/pvh/vpmu: Disable VPMU for PVH guests

[Boris Ostrovsky]

On 11/25/2014 09:55 AM, Jan Beulich wrote:
>
>> Regardless, do you think that disabling VPMU for PVH is worth anyway?
> That depends on what (bad) consequences not doing so has.

I haven't seen anything (besides VAPIC accesses) but I think it would be 
prudent to prevent any VPMU activity from happening. I can see, for 
example MSRs and APIC vector being written. All of which look benign on 
the first sight but who knows...

-boris

Re: [PATCH for-xen-4.5] x86/pvh/vpmu: Disable VPMU for PVH guests

[Jan Beulich]

>>> On 25.11.14 at 17:19, <boris.ostrovsky@oracle.com> wrote:
> On 11/25/2014 09:55 AM, Jan Beulich wrote:
>>
>>> Regardless, do you think that disabling VPMU for PVH is worth anyway?
>> That depends on what (bad) consequences not doing so has.
> 
> I haven't seen anything (besides VAPIC accesses) but I think it would be 
> prudent to prevent any VPMU activity from happening. I can see, for 
> example MSRs and APIC vector being written. All of which look benign on 
> the first sight but who knows...

Yeah, it's not really a problem to put it in (if Konrad agrees; remember
that PVH is still experimental, and hence fixing bugs caused only by it
may be out of scope at this point - in any event I think that if your
patch is to go in, mine should too).

Jan

Re: [PATCH for-xen-4.5] x86/pvh/vpmu: Disable VPMU for PVH guests

[Konrad Rzeszutek Wilk]

On Tue, Nov 25, 2014 at 04:39:54PM +0000, Jan Beulich wrote:
> >>> On 25.11.14 at 17:19, <boris.ostrovsky@oracle.com> wrote:
> > On 11/25/2014 09:55 AM, Jan Beulich wrote:
> >>
> >>> Regardless, do you think that disabling VPMU for PVH is worth anyway?
> >> That depends on what (bad) consequences not doing so has.
> > 
> > I haven't seen anything (besides VAPIC accesses) but I think it would be 
> > prudent to prevent any VPMU activity from happening. I can see, for 
> > example MSRs and APIC vector being written. All of which look benign on 
> > the first sight but who knows...
> 
> Yeah, it's not really a problem to put it in (if Konrad agrees; remember
> that PVH is still experimental, and hence fixing bugs caused only by it
> may be out of scope at this point - in any event I think that if your
> patch is to go in, mine should too).

The beaty of experimental is that we can add it later in the cycle as
at worst they will regress something that is unbaked already.

>From that perspective the bar to put fixes for 'experimental' is lower
than normal code. The part that I am worried about is the common paths
and this potentially causing regressions on the those.

But the potential for that is low that I am OK with these patches
going in. 
> 
> Jan
>