[PATCH 0/4] Permit fault-less access to non-emulated MSRs

[PATCH 0/4] Permit fault-less access to non-emulated MSRs

[Boris Ostrovsky]

Disallowing accesses to MSRs that are not explicitly handled (done by
commit 84e848fd7a16 ("x86/hvm: disallow access to unknown MSRs")) caused
regression for Solaris guests who access MSR_RAPL_POWER_UNIT assuming
it's always there.

We can add special handling for this and other RAPL registers (and that's
what should happen for pre-4.14 releases) but a more general solution is to
provide an option to allow accesses to unhandled registers to proceed.

The new option is "ignore_msrs" and it can take three values
    never:   Issue a warning to the log and #GP to the guest. This is default.
    silent:  MSR reads return 0, MSR writes are ignored. No warnings to the log.
    verbose: Similar to silent but a warning is written.


Boris Ostrovsky (4):
  xl: Add support for ignore_msrs option
  x86: Introduce MSR_UNHANDLED
  x86: Allow non-faulting accesses to non-emulated MSRs if policy
    permits this
  tools/libs: Apply MSR policy to a guest

 docs/man/xl.cfg.5.pod.in          |  20 ++++++-
 tools/include/xenctrl.h           |   2 +
 tools/libs/guest/Makefile         |   1 +
 tools/libs/guest/xg_msrs_x86.c    | 110 ++++++++++++++++++++++++++++++++++++++
 tools/libs/light/libxl_dom.c      |   5 +-
 tools/libs/light/libxl_internal.h |   2 +
 tools/libs/light/libxl_types.idl  |   7 +++
 tools/libs/light/libxl_x86.c      |   7 +++
 tools/xl/xl_parse.c               |   7 +++
 xen/arch/x86/hvm/svm/svm.c        |  10 ++--
 xen/arch/x86/hvm/vmx/vmx.c        |  10 ++--
 xen/arch/x86/msr.c                |  20 +++++++
 xen/arch/x86/pv/emul-priv-op.c    |   8 +--
 xen/include/asm-x86/msr.h         |   3 +-
 xen/include/xen/lib/x86/msr.h     |  17 +++++-
 xen/lib/x86/msr.c                 |  16 +++---
 16 files changed, 217 insertions(+), 28 deletions(-)
 create mode 100644 tools/libs/guest/xg_msrs_x86.c

-- 
1.8.3.1

[PATCH 1/4] xl: Add support for ignore_msrs option

[Boris Ostrovsky]

This option allows guest administrator specify what should happen when
guest accesses an MSR which is not explicitly emulated by the hypervisor.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 docs/man/xl.cfg.5.pod.in         | 20 +++++++++++++++++++-
 tools/libs/light/libxl_types.idl |  7 +++++++
 tools/xl/xl_parse.c              |  7 +++++++
 3 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index c8e017f950de..96ce97c42cab 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -2044,7 +2044,25 @@ Do not provide a VM generation ID.
 See also "Virtual Machine Generation ID" by Microsoft:
 L<https://docs.microsoft.com/en-us/windows/win32/hyperv_v2/virtual-machine-generation-identifier>
 
-=back 
+=over
+
+=item B<ignore_msrs="STRING">
+
+Determine hypervisor behavior on accesses to MSRs that are not emulated by the hypervisor.
+
+=over 4
+
+=item B<never>
+
+Issue a warning to the log and #GP to the guest. This is default.
+
+=item B<silent>
+
+MSR reads return 0, MSR writes are ignored. No warnings to the log.
+
+=item B<verbose>
+
+Similar to B<silent> but a warning is written.
 
 =head3 Guest Virtual Time Controls
 
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 05324736b744..7b5fef771ee8 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -477,6 +477,12 @@ libxl_tee_type = Enumeration("tee_type", [
     (1, "optee")
     ], init_val = "LIBXL_TEE_TYPE_NONE")
 
+libxl_ignore_msrs = Enumeration("ignore_msrs", [
+    (0, "never"),
+    (1, "silent"),
+    (2, "verbose"),
+    ], init_val = "LIBXL_IGNORE_MSRS_NEVER")
+
 libxl_rdm_reserve = Struct("rdm_reserve", [
     ("strategy",    libxl_rdm_reserve_strategy),
     ("policy",      libxl_rdm_reserve_policy),
@@ -559,6 +565,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
     ("apic",             libxl_defbool),
     ("dm_restrict",      libxl_defbool),
     ("tee",              libxl_tee_type),
+    ("ignore_msrs",      libxl_ignore_msrs),
     ("u", KeyedUnion(None, libxl_domain_type, "type",
                 [("hvm", Struct(None, [("firmware",         string),
                                        ("bios",             libxl_bios_type),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 4ebf39620ae7..942086c3f41d 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2732,6 +2732,13 @@ skip_usbdev:
         }
     }
 
+    if (!xlu_cfg_get_string(config, "ignore_msrs", &buf, 0)) {
+        if (libxl_ignore_msrs_from_string(buf, &b_info->ignore_msrs)) {
+           fprintf(stderr, "ERROR: invalid value \"%s\" for \"ignore_msrs\"\n", buf);
+           exit(1);
+        }
+    }
+
     parse_vkb_list(config, d_config);
 
     xlu_cfg_get_defbool(config, "xend_suspend_evtchn_compat",
-- 
1.8.3.1

[PATCH 2/4] x86: Introduce MSR_UNHANDLED

[Boris Ostrovsky]

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

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 xen/include/xen/lib/x86/msr.h | 17 ++++++++++++++++-
 xen/lib/x86/msr.c             | 16 +++++++++-------
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/xen/include/xen/lib/x86/msr.h b/xen/include/xen/lib/x86/msr.h
index 48ba4a59c036..7911ae31eb48 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 -1
+
 /* 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..cf53768dfa4e 100644
--- a/xen/lib/x86/msr.c
+++ b/xen/lib/x86/msr.c
@@ -6,11 +6,11 @@
  * Copy a single MSR into the provided msr_entry_buffer_t buffer, performing a
  * boundary check against the buffer size.
  */
-static int copy_msr_to_buffer(uint32_t idx, uint64_t val,
+static int copy_msr_to_buffer(uint32_t idx, uint64_t val, uint32_t flags,
                               msr_entry_buffer_t msrs,
                               uint32_t *curr_entry, const uint32_t nr_entries)
 {
-    const xen_msr_entry_t ent = { .idx = idx, .val = val };
+    const xen_msr_entry_t ent = { .idx = idx, .val = val, .flags = flags };
 
     if ( *curr_entry == nr_entries )
         return -ENOBUFS;
@@ -29,17 +29,18 @@ int x86_msr_copy_to_buffer(const struct msr_policy *p,
     const uint32_t nr_entries = *nr_entries_p;
     uint32_t curr_entry = 0;
 
-#define COPY_MSR(idx, val)                                      \
+#define COPY_MSR(idx, val, flags)                                      \
     ({                                                          \
         int ret;                                                \
                                                                 \
         if ( (ret = copy_msr_to_buffer(                         \
-                  idx, val, msrs, &curr_entry, nr_entries)) )   \
+                  idx, val, flags, msrs, &curr_entry, nr_entries)) )   \
             return ret;                                         \
     })
 
-    COPY_MSR(MSR_INTEL_PLATFORM_INFO, p->platform_info.raw);
-    COPY_MSR(MSR_ARCH_CAPABILITIES,   p->arch_caps.raw);
+    COPY_MSR(MSR_INTEL_PLATFORM_INFO, p->platform_info.raw, 0);
+    COPY_MSR(MSR_ARCH_CAPABILITIES,   p->arch_caps.raw, 0);
+    COPY_MSR(MSR_UNHANDLED, 0, p->ignore_msrs);
 
 #undef COPY_MSR
 
@@ -77,7 +78,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
         if ( copy_from_buffer_offset(&data, msrs, i, 1) )
             return -EFAULT;
 
-        if ( data.flags ) /* .flags MBZ */
+        if ( data.idx != MSR_UNHANDLED && data.flags )
         {
             rc = -EINVAL;
             goto err;
@@ -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:           p->ignore_msrs = data.flags & 0xff;       break;
 
 #undef ASSIGN
 
-- 
1.8.3.1

Re: [PATCH 2/4] x86: Introduce MSR_UNHANDLED

[Jan Beulich]

On 07.01.2021 21:34, Boris Ostrovsky wrote:
> --- 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:

What about ones handled by emulation, when emulation decides to
raise #GP and this still causes trouble to a guest? (Slightly
orthogonal, so we may want to defer this aspect.)

> + *  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 -1

Not sure about this choice: I'd consider an MSR index in the Xen
range more safe to use, not the least because this value
effectively becomes part of the migration ABI. Or if you use one
outside, then maybe a less prominent one than 0xffffffff (I
guess -1, irrespective of the missing parentheses, isn't
appropriate to use here).

> @@ -77,7 +78,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
>          if ( copy_from_buffer_offset(&data, msrs, i, 1) )
>              return -EFAULT;
>  
> -        if ( data.flags ) /* .flags MBZ */
> +        if ( data.idx != MSR_UNHANDLED && data.flags )

You permit all flags bits set here, but ...

> @@ -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:           p->ignore_msrs = data.flags & 0xff;       break;

... you use only the low 8 ones here. You want to forbid any we
don't know of, and even restrict the low two ones to just the
three values you define. E.g. for now

        if ( data.idx != MSR_UNHANDLED ? data.flags
                                       : data.flags > MSR_UNHANDLED_VERBOSE )
        {
            rc = -EINVAL;
            goto err;

Otoh I don't see why you need to use flags here - I think you
could as well use the MSR value to convey the setting. And if
you don't, you'd want to check the value to be zero.

Nit: You can shorten line length by omitting the "& 0xff" and
reducing the number of blanks ahead of "break;".

Jan

Re: [PATCH 2/4] x86: Introduce MSR_UNHANDLED

[boris.ostrovsky]

On 1/8/21 9:55 AM, Jan Beulich wrote:
> On 07.01.2021 21:34, Boris Ostrovsky wrote:
>> --- 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:
> What about ones handled by emulation, when emulation decides to
> raise #GP and this still causes trouble to a guest? (Slightly
> orthogonal, so we may want to defer this aspect.)


Yes, that's a good point --- in some cases the situation is somewhat similar, e.g. when a new bit shows up in an MSR that until now was considered invalid.


>
>> + *  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 -1
> Not sure about this choice: I'd consider an MSR index in the Xen
> range more safe to use, not the least because this value
> effectively becomes part of the migration ABI. Or if you use one
> outside, then maybe a less prominent one than 0xffffffff (I
> guess -1, irrespective of the missing parentheses, isn't
> appropriate to use here).


All MSRs in Xen range are currently handled (although most return an exception). I can reserve the last one (0x400002ff) if you feel it's more appropriate.


>
>> @@ -77,7 +78,7 @@ int x86_msr_copy_from_buffer(struct msr_policy *p,
>>          if ( copy_from_buffer_offset(&data, msrs, i, 1) )
>>              return -EFAULT;
>>  
>> -        if ( data.flags ) /* .flags MBZ */
>> +        if ( data.idx != MSR_UNHANDLED && data.flags )
> You permit all flags bits set here, but ...
>
>> @@ -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:           p->ignore_msrs = data.flags & 0xff;       break;
> ... you use only the low 8 ones here. You want to forbid any we
> don't know of, and even restrict the low two ones to just the
> three values you define. E.g. for now
>
>         if ( data.idx != MSR_UNHANDLED ? data.flags
>                                        : data.flags > MSR_UNHANDLED_VERBOSE )
>         {
>             rc = -EINVAL;
>             goto err;
>
> Otoh I don't see why you need to use flags here - I think you
> could as well use the MSR value to convey the setting. And if
> you don't, you'd want to check the value to be zero.


Sure, using value will result in a slightly smaller diffstat too.



-boris

Re: [PATCH 2/4] x86: Introduce MSR_UNHANDLED

[Jan Beulich]

On 08.01.2021 21:31, boris.ostrovsky@oracle.com wrote:
> On 1/8/21 9:55 AM, Jan Beulich wrote:
>> On 07.01.2021 21:34, Boris Ostrovsky wrote:
>>> + *  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 -1
>> Not sure about this choice: I'd consider an MSR index in the Xen
>> range more safe to use, not the least because this value
>> effectively becomes part of the migration ABI. Or if you use one
>> outside, then maybe a less prominent one than 0xffffffff (I
>> guess -1, irrespective of the missing parentheses, isn't
>> appropriate to use here).
> 
> 
> All MSRs in Xen range are currently handled (although most return
> an exception). I can reserve the last one (0x400002ff) if you feel
> it's more appropriate.

I do, yes, but I'd prefer to also have Andrew's general view here.
Difficulty is his email delivery issue, so I don't know how soon
we could hope for a reply.

Jan

[PATCH 3/4] x86: Allow non-faulting accesses to non-emulated MSRs if policy permits this

[Boris Ostrovsky]

Starting with commit 84e848fd7a16 ("x86/hvm: disallow access to unknown MSRs")
accesses to unhandled MSRs result in #GP sent to the guest. This caused a
regression for Solaris who tries to acccess MSR_RAPL_POWER_UNIT and (unlike,
for example, Linux) does not catch exceptions when accessing MSRs that
potentially may not be present.

Instead of special-casing RAPL registers we decide what to do when any
non-emulated MSR is accessed based on ignore_msrs field of msr_policy.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---

 xen/arch/x86/hvm/svm/svm.c     | 10 ++++------
 xen/arch/x86/hvm/vmx/vmx.c     | 10 ++++------
 xen/arch/x86/msr.c             | 20 ++++++++++++++++++++
 xen/arch/x86/pv/emul-priv-op.c |  8 ++++----
 xen/include/asm-x86/msr.h      |  3 ++-
 5 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index b819897a4a9f..c9a93448f071 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1965,8 +1965,8 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
         break;
 
     default:
-        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
-        goto gpf;
+        if ( guest_unhandled_msr(v, msr, msr_content, false) )
+            goto gpf;
     }
 
     HVM_DBG_LOG(DBG_LEVEL_MSR, "returns: ecx=%x, msr_value=%"PRIx64,
@@ -2151,10 +2151,8 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
         break;
 
     default:
-        gdprintk(XENLOG_WARNING,
-                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
-                 msr, msr_content);
-        goto gpf;
+        if ( guest_unhandled_msr(v, msr, &msr_content, true) )
+            goto gpf;
     }
 
     return X86EMUL_OKAY;
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index 2d4475ee3de2..34524c7a6f00 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3017,8 +3017,8 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
             break;
         }
 
-        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
-        goto gp_fault;
+        if ( guest_unhandled_msr(curr, msr, msr_content, false) )
+            goto gp_fault;
     }
 
 done:
@@ -3319,10 +3319,8 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
              is_last_branch_msr(msr) )
             break;
 
-        gdprintk(XENLOG_WARNING,
-                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
-                 msr, msr_content);
-        goto gp_fault;
+        if ( guest_unhandled_msr(v, msr, &msr_content, true) )
+            goto gp_fault;
     }
 
     return X86EMUL_OKAY;
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index be8e36386250..e624fc8694bf 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -164,6 +164,26 @@ int init_vcpu_msr_policy(struct vcpu *v)
     return 0;
 }
 
+/* Returns true if policy requires #GP to the guest. */
+bool guest_unhandled_msr(const struct vcpu *v, uint32_t msr,
+                         uint64_t *val, bool is_write)
+{
+    const struct msr_policy *mp = v->domain->arch.msr;
+
+    if ( unlikely(mp->ignore_msrs != MSR_UNHANDLED_NEVER) && !is_write )
+        *val = 0;
+
+    if ( likely(mp->ignore_msrs != MSR_UNHANDLED_SILENT) ) {
+        if ( is_write )
+            gdprintk(XENLOG_WARNING, "WRMSR 0x%08x val 0x%016"PRIx64
+                    " unimplemented\n", msr, *val);
+        else
+            gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
+    }
+
+    return (mp->ignore_msrs == MSR_UNHANDLED_NEVER);
+}
+
 int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val)
 {
     const struct vcpu *curr = current;
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index dbceed8a05fd..96d90eb30adc 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -984,7 +984,8 @@ static int read_msr(unsigned int reg, uint64_t *val,
         }
         /* fall through */
     default:
-        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", reg);
+        if ( !guest_unhandled_msr(curr, reg, val, false) )
+            return X86EMUL_OKAY;
         break;
 
     normal:
@@ -1146,9 +1147,8 @@ static int write_msr(unsigned int reg, uint64_t val,
         }
         /* fall through */
     default:
-        gdprintk(XENLOG_WARNING,
-                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
-                 reg, val);
+        if ( !guest_unhandled_msr(curr, reg, &val, true) )
+            return X86EMUL_OKAY;
         break;
 
     invalid:
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index 16f95e734428..43a48e1a50ce 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -345,5 +345,6 @@ int init_vcpu_msr_policy(struct vcpu *v);
  */
 int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val);
 int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val);
-
+bool guest_unhandled_msr(const struct vcpu *v, uint32_t msr,
+                         uint64_t *val, bool is_write);
 #endif /* __ASM_MSR_H */
-- 
1.8.3.1

Re: [PATCH 3/4] x86: Allow non-faulting accesses to non-emulated MSRs if policy permits this

[Jan Beulich]

On 07.01.2021 21:34, Boris Ostrovsky wrote:
> Starting with commit 84e848fd7a16 ("x86/hvm: disallow access to unknown MSRs")
> accesses to unhandled MSRs result in #GP sent to the guest. This caused a
> regression for Solaris who tries to acccess MSR_RAPL_POWER_UNIT and (unlike,

Nit: One c too many.

> for example, Linux) does not catch exceptions when accessing MSRs that
> potentially may not be present.

Just to re-raise the question raised by Andrew already earlier
on: Has Solaris been fixed in the meantime, or is this at least
firmly planned to happen?

> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -1965,8 +1965,8 @@ static int svm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>          break;
>  
>      default:
> -        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
> -        goto gpf;
> +        if ( guest_unhandled_msr(v, msr, msr_content, false) )
> +            goto gpf;
>      }
>  
>      HVM_DBG_LOG(DBG_LEVEL_MSR, "returns: ecx=%x, msr_value=%"PRIx64,
> @@ -2151,10 +2151,8 @@ static int svm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>          break;
>  
>      default:
> -        gdprintk(XENLOG_WARNING,
> -                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> -                 msr, msr_content);
> -        goto gpf;
> +        if ( guest_unhandled_msr(v, msr, &msr_content, true) )
> +            goto gpf;
>      }
>  
>      return X86EMUL_OKAY;
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -3017,8 +3017,8 @@ static int vmx_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>              break;
>          }
>  
> -        gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", msr);
> -        goto gp_fault;
> +        if ( guest_unhandled_msr(curr, msr, msr_content, false) )
> +            goto gp_fault;
>      }
>  
>  done:
> @@ -3319,10 +3319,8 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>               is_last_branch_msr(msr) )
>              break;
>  
> -        gdprintk(XENLOG_WARNING,
> -                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> -                 msr, msr_content);
> -        goto gp_fault;
> +        if ( guest_unhandled_msr(v, msr, &msr_content, true) )
> +            goto gp_fault;
>      }
>  
>      return X86EMUL_OKAY;

These functions also get used from the insn emulator, when it
needs to fetch an MSR value (not necessarily in the context of
emulating RDMSR or WRMSR). I wonder whether applying this
behavior in that case is actually correct. It would only be if
we would settle on it being a requirement that any such MSRs
have to have emulation present in one of the handlers.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -164,6 +164,26 @@ int init_vcpu_msr_policy(struct vcpu *v)
>      return 0;
>  }
>  
> +/* Returns true if policy requires #GP to the guest. */
> +bool guest_unhandled_msr(const struct vcpu *v, uint32_t msr,
> +                         uint64_t *val, bool is_write)
> +{
> +    const struct msr_policy *mp = v->domain->arch.msr;
> +
> +    if ( unlikely(mp->ignore_msrs != MSR_UNHANDLED_NEVER) && !is_write )
> +        *val = 0;

I'd recommend to drop the left side of the && - there's no harm
in storing zero in this extra case.

> +    if ( likely(mp->ignore_msrs != MSR_UNHANDLED_SILENT) ) {

Nit: Brace on its own line please.

> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -345,5 +345,6 @@ int init_vcpu_msr_policy(struct vcpu *v);
>   */
>  int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val);
>  int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val);
> -
> +bool guest_unhandled_msr(const struct vcpu *v, uint32_t msr,
> +                         uint64_t *val, bool is_write);
>  #endif /* __ASM_MSR_H */

Please retain the blank line that was there.

Jan

Re: [PATCH 3/4] x86: Allow non-faulting accesses to non-emulated MSRs if policy permits this

[boris.ostrovsky]

On 1/8/21 10:18 AM, Jan Beulich wrote:
> On 07.01.2021 21:34, Boris Ostrovsky wrote:
>> Starting with commit 84e848fd7a16 ("x86/hvm: disallow access to unknown MSRs")
>> accesses to unhandled MSRs result in #GP sent to the guest. This caused a
>> regression for Solaris who tries to acccess MSR_RAPL_POWER_UNIT and (unlike,
> Nit: One c too many.
>
>> for example, Linux) does not catch exceptions when accessing MSRs that
>> potentially may not be present.
> Just to re-raise the question raised by Andrew already earlier
> on: Has Solaris been fixed in the meantime, or is this at least
> firmly planned to happen?


I was told they will open a bug.


>  done:
> @@ -3319,10 +3319,8 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>               is_last_branch_msr(msr) )
>              break;
>  
> -        gdprintk(XENLOG_WARNING,
> -                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
> -                 msr, msr_content);
> -        goto gp_fault;
> +        if ( guest_unhandled_msr(v, msr, &msr_content, true) )
> +            goto gp_fault;
>      }
>  
>      return X86EMUL_OKAY;
> These functions also get used from the insn emulator, when it
> needs to fetch an MSR value (not necessarily in the context of
> emulating RDMSR or WRMSR). I wonder whether applying this
> behavior in that case is actually correct. It would only be if
> we would settle on it being a requirement that any such MSRs
> have to have emulation present in one of the handlers.


Hmm.. Yes, I did not consider this. I am not convinced this will always result in correct behavior for the emulator so I will need to pass down a parameter. Unless there is a way to figure out whether we are running in the emulator (which I don't immediately see)


-boris

Re: [PATCH 3/4] x86: Allow non-faulting accesses to non-emulated MSRs if policy permits this

[Jan Beulich]

On 08.01.2021 21:39, boris.ostrovsky@oracle.com wrote:
> On 1/8/21 10:18 AM, Jan Beulich wrote:
>> On 07.01.2021 21:34, Boris Ostrovsky wrote:
>>> Starting with commit 84e848fd7a16 ("x86/hvm: disallow access to unknown MSRs")
>>> accesses to unhandled MSRs result in #GP sent to the guest. This caused a
>>> regression for Solaris who tries to acccess MSR_RAPL_POWER_UNIT and (unlike,
>> Nit: One c too many.
>>
>>> for example, Linux) does not catch exceptions when accessing MSRs that
>>> potentially may not be present.
>> Just to re-raise the question raised by Andrew already earlier
>> on: Has Solaris been fixed in the meantime, or is this at least
>> firmly planned to happen?
> 
> I was told they will open a bug.

"Will", not "did"?

>> @@ -3319,10 +3319,8 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>               is_last_branch_msr(msr) )
>>              break;
>>  
>> -        gdprintk(XENLOG_WARNING,
>> -                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
>> -                 msr, msr_content);
>> -        goto gp_fault;
>> +        if ( guest_unhandled_msr(v, msr, &msr_content, true) )
>> +            goto gp_fault;
>>      }
>>  
>>      return X86EMUL_OKAY;
>> These functions also get used from the insn emulator, when it
>> needs to fetch an MSR value (not necessarily in the context of
>> emulating RDMSR or WRMSR). I wonder whether applying this
>> behavior in that case is actually correct. It would only be if
>> we would settle on it being a requirement that any such MSRs
>> have to have emulation present in one of the handlers.
> 
> 
> Hmm.. Yes, I did not consider this. I am not convinced this will
> always result in correct behavior for the emulator so I will
> need to pass down a parameter. Unless there is a way to figure
> out whether we are running in the emulator (which I don't
> immediately see)

Passing a parameter for this is sort of ugly, but I presume
unavoidable. The more that what you need to know is not "running
in emulator", but "guest RDMSR/WRMSR" - this can also happen
through the emulator.

Jan

Re: [PATCH 3/4] x86: Allow non-faulting accesses to non-emulated MSRs if policy permits this

[boris.ostrovsky]

On 1/11/21 2:48 AM, Jan Beulich wrote:
> On 08.01.2021 21:39, boris.ostrovsky@oracle.com wrote:
>> On 1/8/21 10:18 AM, Jan Beulich wrote:
>>
>>>
>>> Just to re-raise the question raised by Andrew already earlier
>>> on: Has Solaris been fixed in the meantime, or is this at least
>>> firmly planned to happen?
>> I was told they will open a bug.
> "Will", not "did"?


I can't say for sure, I don't have access to their bugDB, they typically keep bugs private (or so I am told). All I can say is that they are aware of this issue.


>
>>> @@ -3319,10 +3319,8 @@ static int vmx_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>>               is_last_branch_msr(msr) )
>>>              break;
>>>  
>>> -        gdprintk(XENLOG_WARNING,
>>> -                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
>>> -                 msr, msr_content);
>>> -        goto gp_fault;
>>> +        if ( guest_unhandled_msr(v, msr, &msr_content, true) )
>>> +            goto gp_fault;
>>>      }
>>>  
>>>      return X86EMUL_OKAY;
>>> These functions also get used from the insn emulator, when it
>>> needs to fetch an MSR value (not necessarily in the context of
>>> emulating RDMSR or WRMSR). I wonder whether applying this
>>> behavior in that case is actually correct. It would only be if
>>> we would settle on it being a requirement that any such MSRs
>>> have to have emulation present in one of the handlers.
>>
>> Hmm.. Yes, I did not consider this. I am not convinced this will
>> always result in correct behavior for the emulator so I will
>> need to pass down a parameter. Unless there is a way to figure
>> out whether we are running in the emulator (which I don't
>> immediately see)
> Passing a parameter for this is sort of ugly, but I presume
> unavoidable. The more that what you need to know is not "running
> in emulator", but "guest RDMSR/WRMSR" - this can also happen
> through the emulator.


Right, that's what I meant.


-boris

[PATCH 4/4] tools/libs: Apply MSR policy to a guest

[Boris Ostrovsky]

When creating a guest, if ignore_msrs option has been specified,
apply it to guest's MSR policy.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---

 tools/include/xenctrl.h           |   2 +
 tools/libs/guest/Makefile         |   1 +
 tools/libs/guest/xg_msrs_x86.c    | 110 ++++++++++++++++++++++++++++++++++++++
 tools/libs/light/libxl_dom.c      |   5 +-
 tools/libs/light/libxl_internal.h |   2 +
 tools/libs/light/libxl_x86.c      |   7 +++
 6 files changed, 125 insertions(+), 2 deletions(-)
 create mode 100644 tools/libs/guest/xg_msrs_x86.c

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 3796425e1eca..1d6a38e73dcf 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1835,6 +1835,8 @@ int xc_cpuid_apply_policy(xc_interface *xch,
                           const uint32_t *featureset,
                           unsigned int nr_features, bool pae, bool itsc,
                           bool nested_virt, const struct xc_xend_cpuid *xend);
+int xc_msr_apply_policy(xc_interface *xch, uint32_t domid,
+                        unsigned int ignore_msr);
 int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
 int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
                         xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libs/guest/Makefile b/tools/libs/guest/Makefile
index 1c729040b337..452155ea0385 100644
--- a/tools/libs/guest/Makefile
+++ b/tools/libs/guest/Makefile
@@ -56,6 +56,7 @@ SRCS-y                 += xg_dom_compat_linux.c
 
 SRCS-$(CONFIG_X86)     += xg_dom_x86.c
 SRCS-$(CONFIG_X86)     += xg_cpuid_x86.c
+SRCS-$(CONFIG_X86)     += xg_msrs_x86.c
 SRCS-$(CONFIG_ARM)     += xg_dom_arm.c
 
 ifeq ($(CONFIG_LIBXC_MINIOS),y)
diff --git a/tools/libs/guest/xg_msrs_x86.c b/tools/libs/guest/xg_msrs_x86.c
new file mode 100644
index 000000000000..464ce9292ad8
--- /dev/null
+++ b/tools/libs/guest/xg_msrs_x86.c
@@ -0,0 +1,110 @@
+/******************************************************************************
+ * xc_msrs_x86.c
+ *
+ * Update MSR policy of a domain.
+ *
+ * Copyright (c) 2021, Oracle and/or its affiliates.
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation;
+ * version 2.1 of the License.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "xc_private.h"
+#include "xen/lib/x86/msr.h"
+
+
+
+int xc_msr_apply_policy(xc_interface *xch, uint32_t domid, unsigned int ignore_msr)
+{
+    int rc;
+    unsigned int nr_leaves, nr_msrs;
+    xen_msr_entry_t *msrs = NULL;
+    struct msr_policy *p = NULL;
+    xc_dominfo_t di;
+    unsigned int err_leaf, err_subleaf, err_msr;
+
+    if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
+         di.domid != domid )
+    {
+        ERROR("Failed to obtain d%d info", domid);
+        rc = -ESRCH;
+        goto out;
+    }
+
+    rc = xc_get_cpu_policy_size(xch, &nr_leaves, &nr_msrs);
+    if ( rc )
+    {
+        PERROR("Failed to obtain policy info size");
+        rc = -errno;
+        goto out;
+    }
+
+    rc = -ENOMEM;
+    if ( (msrs = calloc(nr_msrs, sizeof(*msrs))) == NULL ||
+         (p = calloc(1, sizeof(*p))) == NULL )
+        goto out;
+
+    /* Get the domain's default policy. */
+    nr_leaves = 0;
+    rc = xc_get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+                                              : XEN_SYSCTL_cpu_policy_pv_default,
+                                  &nr_leaves, NULL, &nr_msrs, msrs);
+    if ( rc )
+    {
+        PERROR("Failed to obtain %s default policy", di.hvm ? "hvm" : "pv");
+        rc = -errno;
+        goto out;
+    }
+
+    rc = x86_msr_copy_from_buffer(p, msrs, nr_msrs, &err_msr);
+    if ( rc )
+    {
+        ERROR("Failed to deserialise MSR (err msr %#x) (%d = %s)",
+              err_msr, -rc, strerror(-rc));
+        goto out;
+    }
+
+    p->ignore_msrs = ignore_msr;
+
+    rc = x86_msr_copy_to_buffer(p, msrs, &nr_msrs);
+    if ( rc )
+    {
+        ERROR("Failed to serialise MSR (%d = %s)", -rc, strerror(-rc));
+        goto out;
+    }
+
+    nr_leaves = 0;
+    rc = xc_set_domain_cpu_policy(xch, domid, nr_leaves, NULL, nr_msrs, msrs,
+                                  &err_leaf, &err_subleaf, &err_msr);
+    if ( rc )
+    {
+        PERROR("Failed to set d%d's MSR policy (err leaf %#x, subleaf %#x, msr %#x)",
+               domid, err_leaf, err_subleaf, err_msr);
+        rc = -errno;
+    }
+
+out:
+    free(msrs);
+    free(p);
+
+    return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
+
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
index 19168572fd3e..1f2abf6679d7 100644
--- a/tools/libs/light/libxl_dom.c
+++ b/tools/libs/light/libxl_dom.c
@@ -383,9 +383,10 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     /* Construct a CPUID policy, but only for brand new domains.  Domains
      * being migrated-in/restored have CPUID handled during the
      * static_data_done() callback. */
-    if (!state->restore)
+    if (!state->restore) {
         libxl__cpuid_legacy(ctx, domid, false, info);
-
+        libxl__msr_policy(ctx, domid, info);
+    }
     return rc;
 }
 
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index c79523ba9248..4f369e6a6f14 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -2054,6 +2054,8 @@ _hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
 
 _hidden void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
                                  libxl_domain_build_info *info);
+_hidden void libxl__msr_policy(libxl_ctx *ctx, uint32_t domid,
+                               libxl_domain_build_info *info);
 
 /* Calls poll() again - useful to check whether a signaled condition
  * is still true.  Cannot fail.  Returns currently-true revents. */
diff --git a/tools/libs/light/libxl_x86.c b/tools/libs/light/libxl_x86.c
index 86d272999d67..92ec1da77139 100644
--- a/tools/libs/light/libxl_x86.c
+++ b/tools/libs/light/libxl_x86.c
@@ -1,5 +1,6 @@
 #include "libxl_internal.h"
 #include "libxl_arch.h"
+#include "xen/lib/x86/msr.h"
 
 int libxl__arch_domain_prepare_config(libxl__gc *gc,
                                       libxl_domain_config *d_config,
@@ -838,6 +839,12 @@ int libxl__arch_passthrough_mode_setdefault(libxl__gc *gc,
     return rc;
 }
 
+void libxl__msr_policy(libxl_ctx *ctx, uint32_t domid,
+                       libxl_domain_build_info *info)
+{
+    if (info->ignore_msrs != LIBXL_IGNORE_MSRS_NEVER)
+        xc_msr_apply_policy(ctx->xch, domid, info->ignore_msrs);
+}
 
 /*
  * Local variables:
-- 
1.8.3.1

Re: [PATCH 0/4] Permit fault-less access to non-emulated MSRs

[boris.ostrovsky]

Apparently I can't spell my own name. Please drop it from CC when responding.


-boris