[PATCH] x86: Allow PV guest set X86_CR4_PCE flag

[PATCH] x86: Allow PV guest set X86_CR4_PCE flag

[Boris Ostrovsky]

With added PV support for VPMU, guests may legitimately decide to set
CR4's PCE flag. We should allow this when VPMU is enabled.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/arch/x86/cpu/vpmu.c      | 19 +++++++++++++++++++
 xen/arch/x86/domain.c        | 13 ++++++++++++-
 xen/include/asm-x86/domain.h |  2 ++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 8af3df1..8cac04e 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -81,6 +81,12 @@ static void __init parse_vpmu_param(char *s)
     }
 }
 
+static void update_cr4_mask_pce(bool_t allow)
+{
+    pv_guest_update_cr4_mask(X86_CR4_PCE, 0, allow);
+    pv_guest_update_cr4_mask(X86_CR4_PCE, 1, allow);
+}
+
 void vpmu_lvtpc_update(uint32_t val)
 {
     struct vpmu_struct *vpmu;
@@ -475,6 +481,7 @@ void vpmu_initialise(struct vcpu *v)
             printk(XENLOG_G_WARNING "VPMU: Unknown CPU vendor %d. "
                    "Disabling VPMU\n", vendor);
             opt_vpmu_enabled = 0;
+            update_cr4_mask_pce(0);
             vpmu_mode = XENPMU_MODE_OFF;
         }
         return; /* Don't bother restoring vpmu_count, VPMU is off forever */
@@ -679,7 +686,16 @@ long do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
         if ( (vpmu_count == 0) ||
              ((vpmu_mode ^ pmu_params.val) ==
               (XENPMU_MODE_SELF | XENPMU_MODE_HV)) )
+        {
+            if ( (vpmu_mode != XENPMU_MODE_OFF) &&
+                 (pmu_params.val == XENPMU_MODE_OFF) )
+                update_cr4_mask_pce(0);
+            else if ( (vpmu_mode == XENPMU_MODE_OFF) &&
+                      (pmu_params.val != XENPMU_MODE_OFF) )
+                update_cr4_mask_pce(1);
+
             vpmu_mode = pmu_params.val;
+        }
         else if ( vpmu_mode != pmu_params.val )
         {
             printk(XENLOG_WARNING
@@ -807,8 +823,11 @@ static int __init vpmu_init(void)
     }
 
     if ( vpmu_mode != XENPMU_MODE_OFF )
+    {
+        update_cr4_mask_pce(1);
         printk(XENLOG_INFO "VPMU: version " __stringify(XENPMU_VER_MAJ) "."
                __stringify(XENPMU_VER_MIN) "\n");
+    }
     else
         opt_vpmu_enabled = 0;
 
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 045f6ff..71a2bb3 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -738,7 +738,18 @@ static int __init init_pv_cr4_masks(void)
 
     return 0;
 }
-__initcall(init_pv_cr4_masks);
+presmp_initcall(init_pv_cr4_masks);
+
+void pv_guest_update_cr4_mask(unsigned long mask, bool_t is_compat,
+                              bool_t allow)
+{
+    unsigned long *curr_mask = is_compat ? &compat_pv_cr4_mask : &pv_cr4_mask;
+
+    if ( !allow )
+        *curr_mask |= mask;
+    else
+        *curr_mask &= ~mask;
+}
 
 unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
 {
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 0fce09e..9758b0a 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -561,6 +561,8 @@ void vcpu_show_registers(const struct vcpu *);
 
 /* Clean up CR4 bits that are not under guest control. */
 unsigned long pv_guest_cr4_fixup(const struct vcpu *, unsigned long guest_cr4);
+void pv_guest_update_cr4_mask(unsigned long mask, bool_t is_compat,
+                              bool_t allow);
 
 /* Convert between guest-visible and real CR4 values. */
 #define pv_guest_cr4_to_real_cr4(v)                         \
-- 
1.8.1.4

Re: [PATCH] x86: Allow PV guest set X86_CR4_PCE flag

[Andrew Cooper]

On 10/08/15 15:27, Boris Ostrovsky wrote:
> With added PV support for VPMU, guests may legitimately decide to set
> CR4's PCE flag. We should allow this when VPMU is enabled.
>
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Why?  Even a PV guest using VPMU should know that it doesn't actually
control CR4.PCE

All this (appears to) end up doing is putting PCE into the "allow but
ignore" mask.

How about this (not even compile tested) which is a rather shorter way
of doing the same thing:

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 045f6ff..834ce0f 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -721,10 +721,12 @@ static int __init init_pv_cr4_masks(void)
     unsigned long common_mask = ~X86_CR4_TSD;
 
     /*
-     * All PV guests may attempt to modify TSD, DE and OSXSAVE.
+     * All PV guests may attempt to modify TSD, DE, PCE and OSXSAVE.
      */
     if ( cpu_has_de )
         common_mask &= ~X86_CR4_DE;
+    if ( cpu_has_pce )
+        common_mask &= ~X86_CR4_PCE;
     if ( cpu_has_xsave )
         common_mask &= ~X86_CR4_OSXSAVE;
 
~Andrew

Re: [PATCH] x86: Allow PV guest set X86_CR4_PCE flag

[Boris Ostrovsky]

On 08/10/2015 10:37 AM, Andrew Cooper wrote:
> On 10/08/15 15:27, Boris Ostrovsky wrote:
>> With added PV support for VPMU, guests may legitimately decide to set
>> CR4's PCE flag. We should allow this when VPMU is enabled.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Why?  Even a PV guest using VPMU should know that it doesn't actually
> control CR4.PCE
>
> All this (appears to) end up doing is putting PCE into the "allow but
> ignore" mask.

Yes, that's what I wanted to do.

>
> How about this (not even compile tested) which is a rather shorter way
> of doing the same thing:

We could do this too but I thought that if we have VPMU off there is no 
reason to allow this bit to be set (quietly).

(There is no cpu_has_pce, we'd use cpu_has_arch_perfmon on Intel and do 
this unconditionally on AMD)

-boris

>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 045f6ff..834ce0f 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -721,10 +721,12 @@ static int __init init_pv_cr4_masks(void)
>       unsigned long common_mask = ~X86_CR4_TSD;
>   
>       /*
> -     * All PV guests may attempt to modify TSD, DE and OSXSAVE.
> +     * All PV guests may attempt to modify TSD, DE, PCE and OSXSAVE.
>        */
>       if ( cpu_has_de )
>           common_mask &= ~X86_CR4_DE;
> +    if ( cpu_has_pce )
> +        common_mask &= ~X86_CR4_PCE;
>       if ( cpu_has_xsave )
>           common_mask &= ~X86_CR4_OSXSAVE;
>   
> ~Andrew

Re: [PATCH] x86: Allow PV guest set X86_CR4_PCE flag

[Andrew Cooper]

On 10/08/15 15:49, Boris Ostrovsky wrote:
>
>
> On 08/10/2015 10:37 AM, Andrew Cooper wrote:
>> On 10/08/15 15:27, Boris Ostrovsky wrote:
>>> With added PV support for VPMU, guests may legitimately decide to set
>>> CR4's PCE flag. We should allow this when VPMU is enabled.
>>>
>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> Why?  Even a PV guest using VPMU should know that it doesn't actually
>> control CR4.PCE
>>
>> All this (appears to) end up doing is putting PCE into the "allow but
>> ignore" mask.
>
> Yes, that's what I wanted to do.
>
>>
>> How about this (not even compile tested) which is a rather shorter way
>> of doing the same thing:
>
> We could do this too but I thought that if we have VPMU off there is
> no reason to allow this bit to be set (quietly).

Adding the bit to this mask doesn't allow the guest to play with it. 
Xen never sets CR4.PCE.

The question is whether warning about the guest attempting to set it is
worthwhile or not.  We have no rdpmc support in emulate_privileged_op()
so any attempt to use it will result in a #UD being injected.

>
> (There is no cpu_has_pce, we'd use cpu_has_arch_perfmon on Intel and
> do this unconditionally on AMD)

It could trivially be added.  The salient piece of information is
whether the hardware would support setting CR4.PCE, not whether any of
the interesting features which come with it are present.

~Andrew

Re: [PATCH] x86: Allow PV guest set X86_CR4_PCE flag

[Boris Ostrovsky]

On 08/10/2015 11:02 AM, Andrew Cooper wrote:
> On 10/08/15 15:49, Boris Ostrovsky wrote:
>>
>> On 08/10/2015 10:37 AM, Andrew Cooper wrote:
>>> On 10/08/15 15:27, Boris Ostrovsky wrote:
>>>> With added PV support for VPMU, guests may legitimately decide to set
>>>> CR4's PCE flag. We should allow this when VPMU is enabled.
>>>>
>>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>>> Why?  Even a PV guest using VPMU should know that it doesn't actually
>>> control CR4.PCE
>>>
>>> All this (appears to) end up doing is putting PCE into the "allow but
>>> ignore" mask.
>> Yes, that's what I wanted to do.
>>
>>> How about this (not even compile tested) which is a rather shorter way
>>> of doing the same thing:
>> We could do this too but I thought that if we have VPMU off there is
>> no reason to allow this bit to be set (quietly).
> Adding the bit to this mask doesn't allow the guest to play with it.
> Xen never sets CR4.PCE.
>
> The question is whether warning about the guest attempting to set it is
> worthwhile or not.  We have no rdpmc support in emulate_privileged_op()
> so any attempt to use it will result in a #UD being injected.

That's because (at least on Linux) we turn it into rdmsr.

Actually, let's forget this patch. Given what I just said above, I think 
it's better to fix this on Linux side and just clear PCE bit in 
xen_write_cr4().

-boris

>
>> (There is no cpu_has_pce, we'd use cpu_has_arch_perfmon on Intel and
>> do this unconditionally on AMD)
> It could trivially be added.  The salient piece of information is
> whether the hardware would support setting CR4.PCE, not whether any of
> the interesting features which come with it are present.
>
> ~Andrew