[PATCH 0/3] x86: Further CPUID/MSR cleanup

[PATCH 0/3] x86: Further CPUID/MSR cleanup

[Andrew Cooper]

This is further cleanup work to help the forthcoming toolstack CPUID/MSR work.

Andrew Cooper (3):
  tools/libxc: Drop xc_cpuid_to_str()
  x86/msr: Drop {MISC_ENABLES,PLATFORM_INFO}.available
  x86/msr: Use the architectural layout for MSR_{MISC_ENABLES,PLATFORM_INFO}

 tools/libxc/include/xenctrl.h |  2 --
 tools/libxc/xc_cpuid_x86.c    | 25 +------------------------
 xen/arch/x86/cpu/common.c     |  1 -
 xen/arch/x86/msr.c            | 39 ++++-----------------------------------
 xen/include/asm-x86/msr.h     | 17 +++++++++++------
 5 files changed, 16 insertions(+), 68 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/3] tools/libxc: Drop xc_cpuid_to_str()

[Andrew Cooper]

This helper appears to have been introduced 10 years ago by c/s 5f14a87ceb
"x86, hvm: Guest CPUID configuration" and never had any users at all.

alloc_str() is actually an opencoded calloc(), and now only has a single
caller.  Use calloc() directly and drop alloc_str().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 tools/libxc/include/xenctrl.h |  2 --
 tools/libxc/xc_cpuid_x86.c    | 25 +------------------------
 2 files changed, 1 insertion(+), 26 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 408fa1c..e8285db 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1828,8 +1828,6 @@ int xc_cpuid_apply_policy(xc_interface *xch,
                           uint32_t domid,
                           uint32_t *featureset,
                           unsigned int nr_features);
-void xc_cpuid_to_str(const unsigned int *regs,
-                     char **strs); /* some strs[] may be NULL if ENOMEM */
 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/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 3a168a4..c5c3cdc 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -627,29 +627,6 @@ static int xc_cpuid_do_domctl(
     return do_domctl(xch, &domctl);
 }
 
-static char *alloc_str(void)
-{
-    char *s = malloc(33);
-    if ( s == NULL )
-        return s;
-    memset(s, 0, 33);
-    return s;
-}
-
-void xc_cpuid_to_str(const unsigned int *regs, char **strs)
-{
-    int i, j;
-
-    for ( i = 0; i < 4; i++ )
-    {
-        strs[i] = alloc_str();
-        if ( strs[i] == NULL )
-            continue;
-        for ( j = 0; j < 32; j++ )
-            strs[i][j] = !!((regs[i] & (1U << (31 - j)))) ? '1' : '0';
-    }
-}
-
 static void sanitise_featureset(struct cpuid_domain_info *info)
 {
     const uint32_t fs_size = xc_get_cpu_featureset_size();
@@ -832,7 +809,7 @@ int xc_cpuid_set(
             continue;
         }
         
-        config_transformed[i] = alloc_str();
+        config_transformed[i] = calloc(33, 1); /* 32 bits, NUL terminator. */
         if ( config_transformed[i] == NULL )
         {
             rc = -ENOMEM;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/3] x86/msr: Drop {MISC_ENABLES, PLATFORM_INFO}.available

[Andrew Cooper]

These MSRs are non-architectural and the available booleans were used in lieu
of an architectural signal of availability.  The MSRs are unconditionally
available to HVM guests, but currently for PV guests, are hidden when CPUID
faulting is unavailable.

However, in hindsight, the additional booleans make toolstack MSR interactions
more complicated.  As the behaviour of the MSRs is reserved when unavailable,
unconditionally letting the MSRs be accessible is compatible behaviour, even
for PV guests.

The new behaviour is:
  * PLATFORM_INFO is unconditionally readable even for PV guests and will
    indicate the presense or absense of CPUID Faulting in bit 31.
  * MISC_FEATURES_ENABLES is uncondtionally readable, and bit 0 may be set iff
    PLATFORM_INFO reports that CPUID Faulting is available.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/cpu/common.c |  1 -
 xen/arch/x86/msr.c        | 30 +-----------------------------
 xen/include/asm-x86/msr.h |  2 --
 3 files changed, 1 insertion(+), 32 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 3548b12..b9ff6f8 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -119,7 +119,6 @@ bool __init probe_cpuid_faulting(void)
 	{
 		struct msr_domain_policy *dp = &raw_msr_domain_policy;
 
-		dp->plaform_info.available = true;
 		if (val & MSR_PLATFORM_INFO_CPUID_FAULTING)
 			dp->plaform_info.cpuid_faulting = true;
 	}
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 1e12ccb..6599f10 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -54,35 +54,21 @@ static void __init calculate_host_policy(void)
 static void __init calculate_hvm_max_policy(void)
 {
     struct msr_domain_policy *dp = &hvm_max_msr_domain_policy;
-    struct msr_vcpu_policy *vp = &hvm_max_msr_vcpu_policy;
 
     if ( !hvm_enabled )
         return;
 
     *dp = host_msr_domain_policy;
 
-    /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
     /* It's always possible to emulate CPUID faulting for HVM guests */
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
-         boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
-    {
-        dp->plaform_info.available = true;
-        dp->plaform_info.cpuid_faulting = true;
-    }
-
-    /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
-    vp->misc_features_enables.available = dp->plaform_info.cpuid_faulting;
+    dp->plaform_info.cpuid_faulting = true;
 }
 
 static void __init calculate_pv_max_policy(void)
 {
     struct msr_domain_policy *dp = &pv_max_msr_domain_policy;
-    struct msr_vcpu_policy *vp = &pv_max_msr_vcpu_policy;
 
     *dp = host_msr_domain_policy;
-
-    /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
-    vp->misc_features_enables.available = dp->plaform_info.cpuid_faulting;
 }
 
 void __init init_guest_msr_policy(void)
@@ -107,10 +93,7 @@ int init_domain_msr_policy(struct domain *d)
 
     /* See comment in intel_ctxt_switch_levelling() */
     if ( is_control_domain(d) )
-    {
-        dp->plaform_info.available = false;
         dp->plaform_info.cpuid_faulting = false;
-    }
 
     d->arch.msr = dp;
 
@@ -130,10 +113,6 @@ int init_vcpu_msr_policy(struct vcpu *v)
     *vp = is_pv_domain(d) ? pv_max_msr_vcpu_policy :
                             hvm_max_msr_vcpu_policy;
 
-    /* See comment in intel_ctxt_switch_levelling() */
-    if ( is_control_domain(d) )
-        vp->misc_features_enables.available = false;
-
     v->arch.msr = vp;
 
     return 0;
@@ -160,8 +139,6 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         break;
 
     case MSR_INTEL_PLATFORM_INFO:
-        if ( !dp->plaform_info.available )
-            goto gp_fault;
         *val = (uint64_t)dp->plaform_info.cpuid_faulting <<
                _MSR_PLATFORM_INFO_CPUID_FAULTING;
         break;
@@ -171,8 +148,6 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         goto gp_fault;
 
     case MSR_INTEL_MISC_FEATURES_ENABLES:
-        if ( !vp->misc_features_enables.available )
-            goto gp_fault;
         *val = (uint64_t)vp->misc_features_enables.cpuid_faulting <<
                _MSR_MISC_FEATURES_CPUID_FAULTING;
         break;
@@ -258,9 +233,6 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     {
         bool old_cpuid_faulting = vp->misc_features_enables.cpuid_faulting;
 
-        if ( !vp->misc_features_enables.available )
-            goto gp_fault;
-
         rsvd = ~0ull;
         if ( dp->plaform_info.cpuid_faulting )
             rsvd &= ~MSR_MISC_FEATURES_CPUID_FAULTING;
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index afbeb7f..c28371c 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -262,7 +262,6 @@ struct msr_domain_policy
 {
     /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
     struct {
-        bool available; /* This MSR is non-architectural */
         bool cpuid_faulting;
     } plaform_info;
 };
@@ -290,7 +289,6 @@ struct msr_vcpu_policy
 
     /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
     struct {
-        bool available; /* This MSR is non-architectural */
         bool cpuid_faulting;
     } misc_features_enables;
 };
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/3] x86/msr: Use the architectural layout for MSR_{MISC_ENABLES, PLATFORM_INFO}

[Andrew Cooper]

This simplifies future interactions with the toolstack, by removing the need
for per-MSR custom accessors when shuffling data in/out of a policy.

Use a 32bit raw backing integer (for simplicity), and use a bitfield to move
the cpuid_faulting field to its appropriate position.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>
---
 xen/arch/x86/msr.c        |  9 +++------
 xen/include/asm-x86/msr.h | 15 +++++++++++----
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 6599f10..d035c67 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -139,8 +139,7 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         break;
 
     case MSR_INTEL_PLATFORM_INFO:
-        *val = (uint64_t)dp->plaform_info.cpuid_faulting <<
-               _MSR_PLATFORM_INFO_CPUID_FAULTING;
+        *val = dp->plaform_info.raw;
         break;
 
     case MSR_ARCH_CAPABILITIES:
@@ -148,8 +147,7 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         goto gp_fault;
 
     case MSR_INTEL_MISC_FEATURES_ENABLES:
-        *val = (uint64_t)vp->misc_features_enables.cpuid_faulting <<
-               _MSR_MISC_FEATURES_CPUID_FAULTING;
+        *val = vp->misc_features_enables.raw;
         break;
 
     default:
@@ -240,8 +238,7 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
         if ( val & rsvd )
             goto gp_fault;
 
-        vp->misc_features_enables.cpuid_faulting =
-            val & MSR_MISC_FEATURES_CPUID_FAULTING;
+        vp->misc_features_enables.raw = val;
 
         if ( v == curr && is_hvm_domain(d) && cpu_has_cpuid_faulting &&
              (old_cpuid_faulting ^ vp->misc_features_enables.cpuid_faulting) )
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index c28371c..599ef72 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -261,8 +261,12 @@ static inline void wrmsr_tsc_aux(uint32_t val)
 struct msr_domain_policy
 {
     /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
-    struct {
-        bool cpuid_faulting;
+    union {
+        uint32_t raw;
+        struct {
+            uint32_t :31;
+            bool cpuid_faulting:1;
+        };
     } plaform_info;
 };
 
@@ -288,8 +292,11 @@ struct msr_vcpu_policy
     } spec_ctrl;
 
     /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
-    struct {
-        bool cpuid_faulting;
+    union {
+        uint32_t raw;
+        struct {
+            bool cpuid_faulting:1;
+        };
     } misc_features_enables;
 };
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] tools/libxc: Drop xc_cpuid_to_str()

[Wei Liu]

On Mon, Jul 02, 2018 at 10:57:25AM +0100, Andrew Cooper wrote:
> This helper appears to have been introduced 10 years ago by c/s 5f14a87ceb
> "x86, hvm: Guest CPUID configuration" and never had any users at all.
> 
> alloc_str() is actually an opencoded calloc(), and now only has a single
> caller.  Use calloc() directly and drop alloc_str().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86/msr: Drop {MISC_ENABLES, PLATFORM_INFO}.available

[Sergey Dyasli]

On Mon, 2018-07-02 at 10:57 +0100, Andrew Cooper wrote:
> These MSRs are non-architectural and the available booleans were used in lieu
> of an architectural signal of availability.  The MSRs are unconditionally
> available to HVM guests, but currently for PV guests, are hidden when CPUID
> faulting is unavailable.
> 
> However, in hindsight, the additional booleans make toolstack MSR interactions
> more complicated.  As the behaviour of the MSRs is reserved when unavailable,
> unconditionally letting the MSRs be accessible is compatible behaviour, even
> for PV guests.
> 
> The new behaviour is:
>   * PLATFORM_INFO is unconditionally readable even for PV guests and will
>     indicate the presense or absense of CPUID Faulting in bit 31.
>   * MISC_FEATURES_ENABLES is uncondtionally readable, and bit 0 may be set iff
>     PLATFORM_INFO reports that CPUID Faulting is available.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> 
> diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
> index afbeb7f..c28371c 100644
> --- a/xen/include/asm-x86/msr.h
> +++ b/xen/include/asm-x86/msr.h
> @@ -262,7 +262,6 @@ struct msr_domain_policy
>  {
>      /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
>      struct {
> -        bool available; /* This MSR is non-architectural */
>          bool cpuid_faulting;
>      } plaform_info;
>  };
> @@ -290,7 +289,6 @@ struct msr_vcpu_policy
>  
>      /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
>      struct {
> -        bool available; /* This MSR is non-architectural */
>          bool cpuid_faulting;
>      } misc_features_enables;
>  };

Could you add comments saying that those 2 MSRs are always available
for all guests?

With that, Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] tools/libxc: Drop xc_cpuid_to_str()

[Roger Pau Monné]

On Mon, Jul 02, 2018 at 10:57:25AM +0100, Andrew Cooper wrote:
> This helper appears to have been introduced 10 years ago by c/s 5f14a87ceb
> "x86, hvm: Guest CPUID configuration" and never had any users at all.
> 
> alloc_str() is actually an opencoded calloc(), and now only has a single
> caller.  Use calloc() directly and drop alloc_str().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> @@ -832,7 +809,7 @@ int xc_cpuid_set(
>              continue;
>          }
>          
> -        config_transformed[i] = alloc_str();
> +        config_transformed[i] = calloc(33, 1); /* 32 bits, NUL terminator. */

I would rather do sizeof(*config_transformed[i]), but I'm not going to
insist.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] x86/msr: Use the architectural layout for MSR_{MISC_ENABLES, PLATFORM_INFO}

[Sergey Dyasli]

On Mon, 2018-07-02 at 10:57 +0100, Andrew Cooper wrote:
> This simplifies future interactions with the toolstack, by removing the need
> for per-MSR custom accessors when shuffling data in/out of a policy.
> 
> Use a 32bit raw backing integer (for simplicity), and use a bitfield to move
> the cpuid_faulting field to its appropriate position.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> 

Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

-- 
Thanks,
Sergey
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86/msr: Drop {MISC_ENABLES, PLATFORM_INFO}.available

[Jan Beulich]

>>> On 02.07.18 at 11:57, <andrew.cooper3@citrix.com> wrote:
> These MSRs are non-architectural and the available booleans were used in lieu
> of an architectural signal of availability.  The MSRs are unconditionally
> available to HVM guests, but currently for PV guests, are hidden when CPUID
> faulting is unavailable.
> 
> However, in hindsight, the additional booleans make toolstack MSR interactions
> more complicated.  As the behaviour of the MSRs is reserved when unavailable,

Is it? Isn't the expected (mandated?) behavior raising of #GP(0) in that
case?

> unconditionally letting the MSRs be accessible is compatible behaviour, even
> for PV guests.
> 
> The new behaviour is:
>   * PLATFORM_INFO is unconditionally readable even for PV guests and will
>     indicate the presense or absense of CPUID Faulting in bit 31.
>   * MISC_FEATURES_ENABLES is uncondtionally readable, and bit 0 may be set iff
>     PLATFORM_INFO reports that CPUID Faulting is available.

For these two I agree with the intended new behavior though (despite
seeing a tiny bit of a risk); I merely think the statement above is a little
too broad.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 3/3] x86/msr: Use the architectural layout for MSR_{MISC_ENABLES, PLATFORM_INFO}

[Jan Beulich]

>>> On 02.07.18 at 12:46, <sergey.dyasli@citrix.com> wrote:
> On Mon, 2018-07-02 at 10:57 +0100, Andrew Cooper wrote:
>> This simplifies future interactions with the toolstack, by removing the need
>> for per-MSR custom accessors when shuffling data in/out of a policy.
>> 
>> Use a 32bit raw backing integer (for simplicity), and use a bitfield to move
>> the cpuid_faulting field to its appropriate position.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> 
> 
> Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86/msr: Drop {MISC_ENABLES, PLATFORM_INFO}.available

[Andrew Cooper]

On 02/07/18 12:27, Jan Beulich wrote:
>>>> On 02.07.18 at 11:57, <andrew.cooper3@citrix.com> wrote:
>> These MSRs are non-architectural and the available booleans were used in lieu
>> of an architectural signal of availability.  The MSRs are unconditionally
>> available to HVM guests, but currently for PV guests, are hidden when CPUID
>> faulting is unavailable.
>>
>> However, in hindsight, the additional booleans make toolstack MSR interactions
>> more complicated.  As the behaviour of the MSRs is reserved when unavailable,
> Is it? Isn't the expected (mandated?) behavior raising of #GP(0) in that
> case?

It is platform specific as to whether the MSR exists on not, and the
only reasonable way to find out whether it is available is to try
reading it.

In practice, these MSRs appear to have existed since Netburst, and have
a range of misc non-architectural functionality in them.

These changes do make the MSRs unconditionally available from the guests
point of view, but offering 0's is compatible with how a guest consumes
the information.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/3] x86/msr: Drop {MISC_ENABLES, PLATFORM_INFO}.available

[Roger Pau Monné]

On Mon, Jul 02, 2018 at 10:57:26AM +0100, Andrew Cooper wrote:
> These MSRs are non-architectural and the available booleans were used in lieu
> of an architectural signal of availability.  The MSRs are unconditionally
> available to HVM guests, but currently for PV guests, are hidden when CPUID
> faulting is unavailable.
> 
> However, in hindsight, the additional booleans make toolstack MSR interactions
> more complicated.  As the behaviour of the MSRs is reserved when unavailable,
> unconditionally letting the MSRs be accessible is compatible behaviour, even
> for PV guests.
> 
> The new behaviour is:
>   * PLATFORM_INFO is unconditionally readable even for PV guests and will
>     indicate the presense or absense of CPUID Faulting in bit 31.
>   * MISC_FEATURES_ENABLES is uncondtionally readable, and bit 0 may be set iff
>     PLATFORM_INFO reports that CPUID Faulting is available.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Sergey Dyasli <sergey.dyasli@citrix.com>
> ---
>  xen/arch/x86/cpu/common.c |  1 -
>  xen/arch/x86/msr.c        | 30 +-----------------------------
>  xen/include/asm-x86/msr.h |  2 --
>  3 files changed, 1 insertion(+), 32 deletions(-)
> 
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index 3548b12..b9ff6f8 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -119,7 +119,6 @@ bool __init probe_cpuid_faulting(void)
>  	{
>  		struct msr_domain_policy *dp = &raw_msr_domain_policy;
>  
> -		dp->plaform_info.available = true;
>  		if (val & MSR_PLATFORM_INFO_CPUID_FAULTING)
>  			dp->plaform_info.cpuid_faulting = true;

To reduce code size:

dp->plaform_info.cpuid_faulting = val & MSR_PLATFORM_INFO_CPUID_FAULTING;

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] tools/libxc: Drop xc_cpuid_to_str()

[Andrew Cooper]

On 02/07/18 11:41, Roger Pau Monné wrote:
> On Mon, Jul 02, 2018 at 10:57:25AM +0100, Andrew Cooper wrote:
>> This helper appears to have been introduced 10 years ago by c/s 5f14a87ceb
>> "x86, hvm: Guest CPUID configuration" and never had any users at all.
>>
>> alloc_str() is actually an opencoded calloc(), and now only has a single
>> caller.  Use calloc() directly and drop alloc_str().
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks,

>
>> @@ -832,7 +809,7 @@ int xc_cpuid_set(
>>              continue;
>>          }
>>          
>> -        config_transformed[i] = alloc_str();
>> +        config_transformed[i] = calloc(33, 1); /* 32 bits, NUL terminator. */
> I would rather do sizeof(*config_transformed[i]), but I'm not going to
> insist.

Without a structure of the form:

struct {
    char cfg[33];
};

not amount of sizeof trickery will work here.  Your example reduces to
sizeof(char), rather than 32/33.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/3] x86/msr: Drop {MISC_ENABLES, PLATFORM_INFO}.available

[Andrew Cooper]

These MSRs are non-architectural and the available booleans were used in lieu
of an architectural signal of availability.

However, in hindsight, the additional booleans make toolstack MSR interactions
more complicated.  The MSRs are unconditionally available to HVM guests, but
currently for PV guests, are hidden when CPUID faulting is unavailable.
Instead, switch them to being unconditionally readable, even for PV guests.

The new behaviour is:
  * PLATFORM_INFO is unconditionally readable even for PV guests and will
    indicate the presence or absence of CPUID Faulting in bit 31.
  * MISC_FEATURES_ENABLES is unconditionally readable, and bit 0 may be set
    iff PLATFORM_INFO reports that CPUID Faulting is available.

As a minor bugfix, CPUID Faulting for HVM guests is not restricted to
Intel/AMD hardware.  In particular, VIA have a VT-x implementaion conforming
to the Intel specification.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Sergey Dyasli <sergey.dyasli@citrix.com>

v2:
 * Further reduce code volume in probe_cpuid_faulting()
 * Comments describing the non-architectural behaviour
---
 xen/arch/x86/cpu/common.c |  9 ++-------
 xen/arch/x86/msr.c        | 30 +-----------------------------
 xen/include/asm-x86/msr.h | 19 +++++++++++++++----
 3 files changed, 18 insertions(+), 40 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index aa8a21e..bdd45c3 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -115,13 +115,8 @@ bool __init probe_cpuid_faulting(void)
 	int rc;
 
 	if ((rc = rdmsr_safe(MSR_INTEL_PLATFORM_INFO, val)) == 0)
-	{
-		struct msr_domain_policy *dp = &raw_msr_domain_policy;
-
-		dp->plaform_info.available = true;
-		if (val & MSR_PLATFORM_INFO_CPUID_FAULTING)
-			dp->plaform_info.cpuid_faulting = true;
-	}
+		raw_msr_domain_policy.plaform_info.cpuid_faulting =
+			val & MSR_PLATFORM_INFO_CPUID_FAULTING;
 
 	if (rc ||
 	    !(val & MSR_PLATFORM_INFO_CPUID_FAULTING) ||
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 1e12ccb..6599f10 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -54,35 +54,21 @@ static void __init calculate_host_policy(void)
 static void __init calculate_hvm_max_policy(void)
 {
     struct msr_domain_policy *dp = &hvm_max_msr_domain_policy;
-    struct msr_vcpu_policy *vp = &hvm_max_msr_vcpu_policy;
 
     if ( !hvm_enabled )
         return;
 
     *dp = host_msr_domain_policy;
 
-    /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
     /* It's always possible to emulate CPUID faulting for HVM guests */
-    if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
-         boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
-    {
-        dp->plaform_info.available = true;
-        dp->plaform_info.cpuid_faulting = true;
-    }
-
-    /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
-    vp->misc_features_enables.available = dp->plaform_info.cpuid_faulting;
+    dp->plaform_info.cpuid_faulting = true;
 }
 
 static void __init calculate_pv_max_policy(void)
 {
     struct msr_domain_policy *dp = &pv_max_msr_domain_policy;
-    struct msr_vcpu_policy *vp = &pv_max_msr_vcpu_policy;
 
     *dp = host_msr_domain_policy;
-
-    /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
-    vp->misc_features_enables.available = dp->plaform_info.cpuid_faulting;
 }
 
 void __init init_guest_msr_policy(void)
@@ -107,10 +93,7 @@ int init_domain_msr_policy(struct domain *d)
 
     /* See comment in intel_ctxt_switch_levelling() */
     if ( is_control_domain(d) )
-    {
-        dp->plaform_info.available = false;
         dp->plaform_info.cpuid_faulting = false;
-    }
 
     d->arch.msr = dp;
 
@@ -130,10 +113,6 @@ int init_vcpu_msr_policy(struct vcpu *v)
     *vp = is_pv_domain(d) ? pv_max_msr_vcpu_policy :
                             hvm_max_msr_vcpu_policy;
 
-    /* See comment in intel_ctxt_switch_levelling() */
-    if ( is_control_domain(d) )
-        vp->misc_features_enables.available = false;
-
     v->arch.msr = vp;
 
     return 0;
@@ -160,8 +139,6 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         break;
 
     case MSR_INTEL_PLATFORM_INFO:
-        if ( !dp->plaform_info.available )
-            goto gp_fault;
         *val = (uint64_t)dp->plaform_info.cpuid_faulting <<
                _MSR_PLATFORM_INFO_CPUID_FAULTING;
         break;
@@ -171,8 +148,6 @@ int guest_rdmsr(const struct vcpu *v, uint32_t msr, uint64_t *val)
         goto gp_fault;
 
     case MSR_INTEL_MISC_FEATURES_ENABLES:
-        if ( !vp->misc_features_enables.available )
-            goto gp_fault;
         *val = (uint64_t)vp->misc_features_enables.cpuid_faulting <<
                _MSR_MISC_FEATURES_CPUID_FAULTING;
         break;
@@ -258,9 +233,6 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val)
     {
         bool old_cpuid_faulting = vp->misc_features_enables.cpuid_faulting;
 
-        if ( !vp->misc_features_enables.available )
-            goto gp_fault;
-
         rsvd = ~0ull;
         if ( dp->plaform_info.cpuid_faulting )
             rsvd &= ~MSR_MISC_FEATURES_CPUID_FAULTING;
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index afbeb7f..de441d5 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -260,9 +260,15 @@ static inline void wrmsr_tsc_aux(uint32_t val)
 /* MSR policy object for shared per-domain MSRs */
 struct msr_domain_policy
 {
-    /* 0x000000ce  MSR_INTEL_PLATFORM_INFO */
+    /*
+     * 0x000000ce - MSR_INTEL_PLATFORM_INFO
+     *
+     * This MSR is non-architectural, but for simplicy we allow it to be read
+     * unconditionally.  CPUID Faulting support can be fully emulated for HVM
+     * guests so can be offered unconditionally, while support for PV guests
+     * is dependent on real hardware support.
+     */
     struct {
-        bool available; /* This MSR is non-architectural */
         bool cpuid_faulting;
     } plaform_info;
 };
@@ -288,9 +294,14 @@ struct msr_vcpu_policy
         uint32_t raw;
     } spec_ctrl;
 
-    /* 0x00000140  MSR_INTEL_MISC_FEATURES_ENABLES */
+    /*
+     * 0x00000140 - MSR_INTEL_MISC_FEATURES_ENABLES
+     *
+     * This MSR is non-architectural, but for simplicy we allow it to be read
+     * unconditionally.  The CPUID Faulting bit is the only writeable bit, and
+     * only if enumerated by MSR_PLATFORM_INFO.
+     */
     struct {
-        bool available; /* This MSR is non-architectural */
         bool cpuid_faulting;
     } misc_features_enables;
 };
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] tools/libxc: Drop xc_cpuid_to_str()

[Roger Pau Monné]

On Mon, Jul 02, 2018 at 01:15:10PM +0100, Andrew Cooper wrote:
> On 02/07/18 11:41, Roger Pau Monné wrote:
> > On Mon, Jul 02, 2018 at 10:57:25AM +0100, Andrew Cooper wrote:
> >> This helper appears to have been introduced 10 years ago by c/s 5f14a87ceb
> >> "x86, hvm: Guest CPUID configuration" and never had any users at all.
> >>
> >> alloc_str() is actually an opencoded calloc(), and now only has a single
> >> caller.  Use calloc() directly and drop alloc_str().
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks,
> 
> >
> >> @@ -832,7 +809,7 @@ int xc_cpuid_set(
> >>              continue;
> >>          }
> >>          
> >> -        config_transformed[i] = alloc_str();
> >> +        config_transformed[i] = calloc(33, 1); /* 32 bits, NUL terminator. */
> > I would rather do sizeof(*config_transformed[i]), but I'm not going to
> > insist.
> 
> Without a structure of the form:
> 
> struct {
>     char cfg[33];
> };
> 
> not amount of sizeof trickery will work here.  Your example reduces to
> sizeof(char), rather than 32/33.

I was thinking about using:

config_transformed[i] = calloc(33, *config_transformed[i]);

So just change the last hardcoded '1'.

Although using some kind of structure to convey this information
instead of a plain char array doesn't seem bad IMHO.

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/3] tools/libxc: Drop xc_cpuid_to_str()

[Roger Pau Monné]

On Mon, Jul 02, 2018 at 03:36:42PM +0200, Roger Pau Monné wrote:
> On Mon, Jul 02, 2018 at 01:15:10PM +0100, Andrew Cooper wrote:
> > On 02/07/18 11:41, Roger Pau Monné wrote:
> > > On Mon, Jul 02, 2018 at 10:57:25AM +0100, Andrew Cooper wrote:
> > >> This helper appears to have been introduced 10 years ago by c/s 5f14a87ceb
> > >> "x86, hvm: Guest CPUID configuration" and never had any users at all.
> > >>
> > >> alloc_str() is actually an opencoded calloc(), and now only has a single
> > >> caller.  Use calloc() directly and drop alloc_str().
> > >>
> > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> > 
> > Thanks,
> > 
> > >
> > >> @@ -832,7 +809,7 @@ int xc_cpuid_set(
> > >>              continue;
> > >>          }
> > >>          
> > >> -        config_transformed[i] = alloc_str();
> > >> +        config_transformed[i] = calloc(33, 1); /* 32 bits, NUL terminator. */
> > > I would rather do sizeof(*config_transformed[i]), but I'm not going to
> > > insist.
> > 
> > Without a structure of the form:
> > 
> > struct {
> >     char cfg[33];
> > };
> > 
> > not amount of sizeof trickery will work here.  Your example reduces to
> > sizeof(char), rather than 32/33.
> 
> I was thinking about using:
> 
> config_transformed[i] = calloc(33, *config_transformed[i]);

Er, rather:

config_transformed[i] = calloc(33, sizeof(*config_transformed[i]));

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel