Re: [Xen-devel] [PATCH] x86: Allow PV guest set X86_CR4_PCE flag
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.ostrov...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86: Allow PV guest set X86_CR4_PCE flag
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.ostrov...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86: Allow PV guest set X86_CR4_PCE flag
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.ostrov...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] x86: Allow PV guest set X86_CR4_PCE flag
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.ostrov...@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 ___ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
[Xen-devel] [PATCH] x86: Allow PV guest set X86_CR4_PCE flag
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.ostrov...@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
___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
