When toolstack updates MSR policy, this MSR offset (which is the last
index in the hypervisor MSR range) is used to indicate hypervisor
behavior when guest accesses an MSR which is not explicitly emulated.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
Changes in v2:
* Use 0x400002ff for MSR_UNHANDLED
* Pass ignore_msrs in msr_policy.value

 xen/arch/x86/msr.c            |  4 ++--
 xen/include/xen/lib/x86/msr.h | 17 ++++++++++++++++-
 xen/lib/x86/msr.c             |  2 ++
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index be8e36386250..433d16c80728 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -295,7 +295,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
         }
 
         /* Fallthrough. */
-    case 0x40000200 ... 0x400002ff:
+    case 0x40000200 ... 0x400002fe:
         ret = guest_rdmsr_xen(v, msr, val);
         break;
 
@@ -514,7 +514,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         }
 
         /* Fallthrough. */
-    case 0x40000200 ... 0x400002ff:
+    case 0x40000200 ... 0x400002fe:
         ret = guest_wrmsr_xen(v, msr, val);
         break;
 
diff --git a/xen/include/xen/lib/x86/msr.h b/xen/include/xen/lib/x86/msr.h
index 48ba4a59c036..fbbb3b7ba870 100644
--- a/xen/include/xen/lib/x86/msr.h
+++ b/xen/include/xen/lib/x86/msr.h
@@ -2,8 +2,21 @@
 #ifndef XEN_LIB_X86_MSR_H
 #define XEN_LIB_X86_MSR_H
 
+/*
+ * Behavior on accesses to MSRs that are not handled by emulation:
+ *  0 = return #GP, warning emitted
+ *  1 = read as 0, writes are dropped, no warning
+ *  2 = read as 0, writes are dropped, warning emitted
+ */
+#define MSR_UNHANDLED_NEVER     0
+#define MSR_UNHANDLED_SILENT    1
+#define MSR_UNHANDLED_VERBOSE   2
+
+/* MSR that is not explicitly processed by emulation */
+#define MSR_UNHANDLED           0x400002ff
+
 /* Maximum number of MSRs written when serialising msr_policy. */
-#define MSR_MAX_SERIALISED_ENTRIES 2
+#define MSR_MAX_SERIALISED_ENTRIES 3
 
 /* MSR policy object for shared per-domain MSRs */
 struct msr_policy
@@ -45,6 +58,8 @@ struct msr_policy
             bool taa_no:1;
         };
     } arch_caps;
+
+    uint8_t ignore_msrs;
 };
 
 #ifdef __XEN__
diff --git a/xen/lib/x86/msr.c b/xen/lib/x86/msr.c
index 7d71e92a380a..178203803946 100644
--- a/xen/lib/x86/msr.c
+++ b/xen/lib/x86/msr.c
@@ -40,6 +40,7 @@ int x86_msr_copy_to_buffer(const struct msr_policy *p,
 
     COPY_MSR(MSR_INTEL_PLATFORM_INFO, p->platform_info.raw);
     COPY_MSR(MSR_ARCH_CAPABILITIES,   p->arch_caps.raw);
+    COPY_MSR(MSR_UNHANDLED,           p->ignore_msrs);
 
 #undef COPY_MSR
 
@@ -101,6 +102,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
 
         case MSR_INTEL_PLATFORM_INFO: ASSIGN(platform_info.raw); break;
         case MSR_ARCH_CAPABILITIES:   ASSIGN(arch_caps.raw);     break;
+        case MSR_UNHANDLED:           ASSIGN(ignore_msrs);       break;
 
 #undef ASSIGN
 
-- 
1.8.3.1

On 20.01.2021 23:49, Boris Ostrovsky wrote:
> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -295,7 +295,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>          }
>  
>          /* Fallthrough. */
> -    case 0x40000200 ... 0x400002ff:
> +    case 0x40000200 ... 0x400002fe:
>          ret = guest_rdmsr_xen(v, msr, val);
>          break;
>  
> @@ -514,7 +514,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>          }
>  
>          /* Fallthrough. */
> -    case 0x40000200 ... 0x400002ff:
> +    case 0x40000200 ... 0x400002fe:
>          ret = guest_wrmsr_xen(v, msr, val);
>          break;

For both of these, we need some kind of connection to
MSR_UNHANDLED. Could be a BUILD_BUG_ON() or an explicit
"case MSR_UNHANDLED:" (preventing someone "correcting" the
apparent mistake) or yet something else.

> --- a/xen/include/xen/lib/x86/msr.h
> +++ b/xen/include/xen/lib/x86/msr.h
> @@ -2,8 +2,21 @@
>  #ifndef XEN_LIB_X86_MSR_H
>  #define XEN_LIB_X86_MSR_H
>  
> +/*
> + * Behavior on accesses to MSRs that are not handled by emulation:
> + *  0 = return #GP, warning emitted
> + *  1 = read as 0, writes are dropped, no warning
> + *  2 = read as 0, writes are dropped, warning emitted
> + */
> +#define MSR_UNHANDLED_NEVER     0
> +#define MSR_UNHANDLED_SILENT    1
> +#define MSR_UNHANDLED_VERBOSE   2
> +
> +/* MSR that is not explicitly processed by emulation */
> +#define MSR_UNHANDLED           0x400002ff

MSR indexes as well as definitions for MSR contents generally
live in asm-x86/msr-index.h. I think it would be better for
the above to also go there.

Additionally the comment on MSR_UNHANDLED should not only
say what will not happen for this index, but also what its
intended purpose is.

> @@ -45,6 +58,8 @@ struct msr_policy
>              bool taa_no:1;
>          };
>      } arch_caps;
> +
> +    uint8_t ignore_msrs;

Add a brief comment along the lines of /* MSR_UNHANDLED_* */
here to make the connection to intended values?

Also, Andrew, (I think I did say so before) - I definitely
would want your general consent with this model, as what gets
altered here is almost all relatively recent contributions
by you. Nor would I exclude the approach being controversial.

Jan

On 1/22/21 6:51 AM, Jan Beulich wrote:
> On 20.01.2021 23:49, Boris Ostrovsky wrote:
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -295,7 +295,7 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
>>          }
>>  
>>          /* Fallthrough. */
>> -    case 0x40000200 ... 0x400002ff:
>> +    case 0x40000200 ... 0x400002fe:
>>          ret = guest_rdmsr_xen(v, msr, val);
>>          break;
>>  
>> @@ -514,7 +514,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
>>          }
>>  
>>          /* Fallthrough. */
>> -    case 0x40000200 ... 0x400002ff:
>> +    case 0x40000200 ... 0x400002fe:
>>          ret = guest_wrmsr_xen(v, msr, val);
>>          break;
> 
> For both of these, we need some kind of connection to
> MSR_UNHANDLED. Could be a BUILD_BUG_ON() or an explicit
> "case MSR_UNHANDLED:" (preventing someone "correcting" the
> apparent mistake) or yet something else.


I actually meant to add a comment there but forgot.

I'll add an explicit 'case'.


> 
>> --- a/xen/include/xen/lib/x86/msr.h
>> +++ b/xen/include/xen/lib/x86/msr.h
>> @@ -2,8 +2,21 @@
>>  #ifndef XEN_LIB_X86_MSR_H
>>  #define XEN_LIB_X86_MSR_H
>>  
>> +/*
>> + * Behavior on accesses to MSRs that are not handled by emulation:
>> + *  0 = return #GP, warning emitted
>> + *  1 = read as 0, writes are dropped, no warning
>> + *  2 = read as 0, writes are dropped, warning emitted
>> + */
>> +#define MSR_UNHANDLED_NEVER     0
>> +#define MSR_UNHANDLED_SILENT    1
>> +#define MSR_UNHANDLED_VERBOSE   2
>> +
>> +/* MSR that is not explicitly processed by emulation */
>> +#define MSR_UNHANDLED           0x400002ff
> 
> MSR indexes as well as definitions for MSR contents generally
> live in asm-x86/msr-index.h. I think it would be better for
> the above to also go there.
> 


I didn't put it there because I felt that file is for "real" (including hypervisor range) MSRs. But I can move it to msr-index.h


-boris

On 1/22/21 6:51 AM, Jan Beulich wrote:
> Also, Andrew, (I think I did say so before) - I definitely
> would want your general consent with this model, as what gets
> altered here is almost all relatively recent contributions
> by you. Nor would I exclude the approach being controversial.
>

Andrew, ping?


-boris