Historically when running as Xen PV guest all MSR accesses have been
silently swallowing any GP faults, even when the kernel was using not
the *msr_safe() access functions.
Change that by making the behavior controllable via kernel config and
via a boot parameter.
This will help finding paths where MSRs are being accessed under Xen
which are not emulated by the hypervisor.
Juergen Gross (3):
xen/pv: allow pmu msr accesses to cause GP
xen/pv: refactor msr access functions to support safe and unsafe
accesses
xen/pv: support selecting safe/unsafe msr accesses
.../admin-guide/kernel-parameters.txt | 6 ++
arch/x86/xen/Kconfig | 9 ++
arch/x86/xen/enlighten_pv.c | 99 +++++++++++++------
arch/x86/xen/pmu.c | 61 ++++++------
4 files changed, 118 insertions(+), 57 deletions(-)
--
2.35.3
Today pmu_msr_read() and pmu_msr_write() fall back to the safe variants
of read/write MSR in case the MSR access isn't emulated via Xen. Allow
the caller to select the potentially faulting variant by passing NULL
for the error pointer.
Restructure the code to make it more readable.
Signed-off-by: Juergen Gross <[email protected]>
---
V2:
- do some restructuring (Jan Beulich, Boris Ostrovsky)
---
arch/x86/xen/pmu.c | 61 +++++++++++++++++++++++++---------------------
1 file changed, 33 insertions(+), 28 deletions(-)
diff --git a/arch/x86/xen/pmu.c b/arch/x86/xen/pmu.c
index 21ecbe754cb2..501b6f872d96 100644
--- a/arch/x86/xen/pmu.c
+++ b/arch/x86/xen/pmu.c
@@ -131,6 +131,9 @@ static inline uint32_t get_fam15h_addr(u32 addr)
static inline bool is_amd_pmu_msr(unsigned int msr)
{
+ if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+ return false;
+
if ((msr >= MSR_F15H_PERF_CTL &&
msr < MSR_F15H_PERF_CTR + (amd_num_counters * 2)) ||
(msr >= MSR_K7_EVNTSEL0 &&
@@ -144,6 +147,9 @@ static int is_intel_pmu_msr(u32 msr_index, int *type, int *index)
{
u32 msr_index_pmc;
+ if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
+ return false;
+
switch (msr_index) {
case MSR_CORE_PERF_FIXED_CTR_CTRL:
case MSR_IA32_DS_AREA:
@@ -292,46 +298,45 @@ static bool xen_amd_pmu_emulate(unsigned int msr, u64 *val, bool is_read)
bool pmu_msr_read(unsigned int msr, uint64_t *val, int *err)
{
- if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
- if (is_amd_pmu_msr(msr)) {
- if (!xen_amd_pmu_emulate(msr, val, 1))
- *val = native_read_msr_safe(msr, err);
- return true;
- }
- } else {
- int type, index;
+ int type, index;
+ bool emulated;
- if (is_intel_pmu_msr(msr, &type, &index)) {
- if (!xen_intel_pmu_emulate(msr, val, type, index, 1))
- *val = native_read_msr_safe(msr, err);
- return true;
- }
+ if (is_amd_pmu_msr(msr))
+ emulated = xen_amd_pmu_emulate(msr, val, 1);
+ else if (is_intel_pmu_msr(msr, &type, &index))
+ emulated = xen_intel_pmu_emulate(msr, val, type, index, 1);
+ else
+ return false;
+
+ if (!emulated) {
+ *val = err ? native_read_msr_safe(msr, err)
+ : native_read_msr(msr);
}
- return false;
+ return true;
}
bool pmu_msr_write(unsigned int msr, uint32_t low, uint32_t high, int *err)
{
uint64_t val = ((uint64_t)high << 32) | low;
+ int type, index;
+ bool emulated;
- if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
- if (is_amd_pmu_msr(msr)) {
- if (!xen_amd_pmu_emulate(msr, &val, 0))
- *err = native_write_msr_safe(msr, low, high);
- return true;
- }
- } else {
- int type, index;
+ if (is_amd_pmu_msr(msr))
+ emulated = xen_amd_pmu_emulate(msr, &val, 0);
+ else if (is_intel_pmu_msr(msr, &type, &index))
+ emulated = xen_intel_pmu_emulate(msr, &val, type, index, 0);
+ else
+ return false;
- if (is_intel_pmu_msr(msr, &type, &index)) {
- if (!xen_intel_pmu_emulate(msr, &val, type, index, 0))
- *err = native_write_msr_safe(msr, low, high);
- return true;
- }
+ if (!emulated) {
+ if (err)
+ *err = native_write_msr_safe(msr, low, high);
+ else
+ native_write_msr(msr, low, high);
}
- return false;
+ return true;
}
static unsigned long long xen_amd_read_pmc(int counter)
--
2.35.3
Instead of always doing the safe variants for reading and writing MSRs
in Xen PV guests, make the behavior controllable via Kconfig option
and a boot parameter.
The default will be the current behavior, which is to always use the
safe variant.
Signed-off-by: Juergen Gross <[email protected]>
---
.../admin-guide/kernel-parameters.txt | 6 +++++
arch/x86/xen/Kconfig | 9 +++++++
arch/x86/xen/enlighten_pv.c | 24 +++++++++++--------
3 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 426fa892d311..1bda9cf18fae 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6836,6 +6836,12 @@
Crash from Xen panic notifier, without executing late
panic() code such as dumping handler.
+ xen_msr_safe= [X86,XEN]
+ Format: <bool>
+ Select whether to always use non-faulting (safe) MSR
+ access functions when running as Xen PV guest. The
+ default value is controlled by CONFIG_XEN_PV_MSR_SAFE.
+
xen_nopvspin [X86,XEN]
Disables the qspinlock slowpath using Xen PV optimizations.
This parameter is obsoleted by "nopvspin" parameter, which
diff --git a/arch/x86/xen/Kconfig b/arch/x86/xen/Kconfig
index 85246dd9faa1..9b1ec5d8c99c 100644
--- a/arch/x86/xen/Kconfig
+++ b/arch/x86/xen/Kconfig
@@ -92,3 +92,12 @@ config XEN_DOM0
select X86_X2APIC if XEN_PVH && X86_64
help
Support running as a Xen Dom0 guest.
+
+config XEN_PV_MSR_SAFE
+ bool "Always use safe MSR accesses in PV guests"
+ default y
+ depends on XEN_PV
+ help
+ Use safe (not faulting) MSR access functions even if the MSR access
+ should not fault anyway.
+ The default can be changed by using the "xen_msr_safe" boot parameter.
diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index d5b0844a1b7c..daae454191f2 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -108,6 +108,16 @@ struct tls_descs {
*/
static DEFINE_PER_CPU(struct tls_descs, shadow_tls_desc);
+static __read_mostly bool xen_msr_safe = IS_ENABLED(CONFIG_XEN_PV_MSR_SAFE);
+
+static int __init parse_xen_msr_safe(char *str)
+{
+ if (str)
+ return strtobool(str, &xen_msr_safe);
+ return -EINVAL;
+}
+early_param("xen_msr_safe", parse_xen_msr_safe);
+
static void __init xen_pv_init_platform(void)
{
/* PV guests can't operate virtio devices without grants. */
@@ -1011,22 +1021,16 @@ static int xen_write_msr_safe(unsigned int msr, unsigned int low,
static u64 xen_read_msr(unsigned int msr)
{
- /*
- * This will silently swallow a #GP from RDMSR. It may be worth
- * changing that.
- */
int err;
- return xen_read_msr_safe(msr, &err);
+ return xen_do_read_msr(msr, xen_msr_safe ? &err : NULL);
}
static void xen_write_msr(unsigned int msr, unsigned low, unsigned high)
{
- /*
- * This will silently swallow a #GP from WRMSR. It may be worth
- * changing that.
- */
- xen_write_msr_safe(msr, low, high);
+ int err;
+
+ xen_do_write_msr(msr, low, high, xen_msr_safe ? &err : NULL);
}
/* This is called once we have the cpu_possible_mask */
--
2.35.3
On 04.10.2022 10:43, Juergen Gross wrote:
> Today pmu_msr_read() and pmu_msr_write() fall back to the safe variants
> of read/write MSR in case the MSR access isn't emulated via Xen. Allow
> the caller to select the potentially faulting variant by passing NULL
> for the error pointer.
>
> Restructure the code to make it more readable.
>
> Signed-off-by: Juergen Gross <[email protected]>
I think the title (and to some degree also the description) is misleading:
The property we care about here isn't whether an MSR access would raise
#GP (we can't control that), but whether that #GP would be recovered from.
> --- a/arch/x86/xen/pmu.c
> +++ b/arch/x86/xen/pmu.c
> @@ -131,6 +131,9 @@ static inline uint32_t get_fam15h_addr(u32 addr)
>
> static inline bool is_amd_pmu_msr(unsigned int msr)
> {
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> + return false;
I understand this and ...
> @@ -144,6 +147,9 @@ static int is_intel_pmu_msr(u32 msr_index, int *type, int *index)
> {
> u32 msr_index_pmc;
>
> + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
> + return false;
... this matches prior behavior, but may I suggest that while moving
these here you at least accompany them by a comment clarifying that
these aren't really correct? We'd come closer if is_amd_pmu_msr()
accepted AMD and Hygon, while is_intel_pmu_msr() may want to accept
Intel and Centaur (but I understand this would be largely orthogonal,
hence the suggestion towards comments). In the hypervisor we kind of
also support Shanghai, but I wonder whether we wouldn't better rip
out that code as unmaintained.
Jan
On 04.10.2022 17:22, Juergen Gross wrote:
> On 04.10.22 12:58, Jan Beulich wrote:
>> On 04.10.2022 10:43, Juergen Gross wrote:
>>> Today pmu_msr_read() and pmu_msr_write() fall back to the safe variants
>>> of read/write MSR in case the MSR access isn't emulated via Xen. Allow
>>> the caller to select the potentially faulting variant by passing NULL
>>> for the error pointer.
>>>
>>> Restructure the code to make it more readable.
>>>
>>> Signed-off-by: Juergen Gross <[email protected]>
>>
>> I think the title (and to some degree also the description) is misleading:
>> The property we care about here isn't whether an MSR access would raise
>> #GP (we can't control that), but whether that #GP would be recovered from.
>
> Would you be fine with adding "fatal" or "visible"?
That would help, but "allow" also is a little odd when it comes to
(likely) crashing the system.
Jan
On 04.10.22 12:58, Jan Beulich wrote:
> On 04.10.2022 10:43, Juergen Gross wrote:
>> Today pmu_msr_read() and pmu_msr_write() fall back to the safe variants
>> of read/write MSR in case the MSR access isn't emulated via Xen. Allow
>> the caller to select the potentially faulting variant by passing NULL
>> for the error pointer.
>>
>> Restructure the code to make it more readable.
>>
>> Signed-off-by: Juergen Gross <[email protected]>
>
> I think the title (and to some degree also the description) is misleading:
> The property we care about here isn't whether an MSR access would raise
> #GP (we can't control that), but whether that #GP would be recovered from.
Would you be fine with adding "fatal" or "visible"?
>
>> --- a/arch/x86/xen/pmu.c
>> +++ b/arch/x86/xen/pmu.c
>> @@ -131,6 +131,9 @@ static inline uint32_t get_fam15h_addr(u32 addr)
>>
>> static inline bool is_amd_pmu_msr(unsigned int msr)
>> {
>> + if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
>> + return false;
>
> I understand this and ...
>
>> @@ -144,6 +147,9 @@ static int is_intel_pmu_msr(u32 msr_index, int *type, int *index)
>> {
>> u32 msr_index_pmc;
>>
>> + if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)
>> + return false;
>
> ... this matches prior behavior, but may I suggest that while moving
> these here you at least accompany them by a comment clarifying that
> these aren't really correct? We'd come closer if is_amd_pmu_msr()
> accepted AMD and Hygon, while is_intel_pmu_msr() may want to accept
> Intel and Centaur (but I understand this would be largely orthogonal,
> hence the suggestion towards comments). In the hypervisor we kind of
> also support Shanghai, but I wonder whether we wouldn't better rip
> out that code as unmaintained.
Maybe the correct thing to do would be to add another patch to fix
is_*_pmu_msr() along the lines you are suggesting.
Juergen
On 10/4/22 4:43 AM, Juergen Gross wrote:
>
> bool pmu_msr_read(unsigned int msr, uint64_t *val, int *err)
> {
> - if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL) {
> - if (is_amd_pmu_msr(msr)) {
> - if (!xen_amd_pmu_emulate(msr, val, 1))
> - *val = native_read_msr_safe(msr, err);
> - return true;
> - }
> - } else {
> - int type, index;
> + int type, index;
> + bool emulated;
>
> - if (is_intel_pmu_msr(msr, &type, &index)) {
> - if (!xen_intel_pmu_emulate(msr, val, type, index, 1))
> - *val = native_read_msr_safe(msr, err);
> - return true;
> - }
> + if (is_amd_pmu_msr(msr))
> + emulated = xen_amd_pmu_emulate(msr, val, 1);
> + else if (is_intel_pmu_msr(msr, &type, &index))
> + emulated = xen_intel_pmu_emulate(msr, val, type, index, 1);
> + else
> + return false;
> +
> + if (!emulated) {
You can factor this out even further I think by moving if/elseif/esle into a separate routine and then have 'if (!xen_emulate_pmu_msr(msr, val, 1))' (and pass zero from pmu_msr_write())
-boris