[01/10] x86/sysctl: Don't return cpu policy data for compiled-out support (2)
diff mbox series

Message ID 20200226202221.6555-2-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86: Default vs Max policies
Related show

Commit Message

Andrew Cooper Feb. 26, 2020, 8:22 p.m. UTC
Just as with c/s 96dc77b4b1 for XEN_SYSCTL_get_cpu_policy,
XEN_SYSCTL_get_cpu_featureset needs to become conditional.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/misc/xen-cpuid.c      | 17 +++++++++++++----
 xen/arch/x86/sysctl.c       | 17 +++++++++++++----
 xen/include/public/sysctl.h |  2 ++
 3 files changed, 28 insertions(+), 8 deletions(-)

Comments

Jan Beulich Feb. 27, 2020, 7:38 a.m. UTC | #1
On 26.02.2020 21:22, Andrew Cooper wrote:
> Just as with c/s 96dc77b4b1 for XEN_SYSCTL_get_cpu_policy,
> XEN_SYSCTL_get_cpu_featureset needs to become conditional.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Albeit I'd say "want", not "needs" in the description.

Jan
Andrew Cooper Feb. 27, 2020, 9:33 a.m. UTC | #2
On 27/02/2020 07:38, Jan Beulich wrote:
> On 26.02.2020 21:22, Andrew Cooper wrote:
>> Just as with c/s 96dc77b4b1 for XEN_SYSCTL_get_cpu_policy,
>> XEN_SYSCTL_get_cpu_featureset needs to become conditional.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Albeit I'd say "want", not "needs" in the description.

It occurs to me that XEN_SYSCTL_get_cpu_featureset is strictly a subset
of XEN_SYSCTL_get_cpu_policy, and that now I've adjusted the toolstack
onto get_cpu_policy, the sole remaining user is xen-cpuid.

get_cpu_policy already has separate default and max indices, whereas
get_cpu_featureset was written before the need for this has become obvious.

This leads to an asymmetry in xen-cpuid, where the -p (policy) option
provides two more sets of information than the featureset listing.

Instead, I think I'd like to drop XEN_SYSCTL_get_cpu_featureset and
update the sole user to the more complete interface.

Thoughts?

~Andrew
Jan Beulich Feb. 27, 2020, 9:40 a.m. UTC | #3
On 27.02.2020 10:33, Andrew Cooper wrote:
> On 27/02/2020 07:38, Jan Beulich wrote:
>> On 26.02.2020 21:22, Andrew Cooper wrote:
>>> Just as with c/s 96dc77b4b1 for XEN_SYSCTL_get_cpu_policy,
>>> XEN_SYSCTL_get_cpu_featureset needs to become conditional.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> Albeit I'd say "want", not "needs" in the description.
> 
> It occurs to me that XEN_SYSCTL_get_cpu_featureset is strictly a subset
> of XEN_SYSCTL_get_cpu_policy, and that now I've adjusted the toolstack
> onto get_cpu_policy, the sole remaining user is xen-cpuid.
> 
> get_cpu_policy already has separate default and max indices, whereas
> get_cpu_featureset was written before the need for this has become obvious.
> 
> This leads to an asymmetry in xen-cpuid, where the -p (policy) option
> provides two more sets of information than the featureset listing.
> 
> Instead, I think I'd like to drop XEN_SYSCTL_get_cpu_featureset and
> update the sole user to the more complete interface.

Sounds like a good move to me.

Jan
Andrew Cooper Feb. 27, 2020, 4:24 p.m. UTC | #4
On 27/02/2020 09:40, Jan Beulich wrote:
> On 27.02.2020 10:33, Andrew Cooper wrote:
>> On 27/02/2020 07:38, Jan Beulich wrote:
>>> On 26.02.2020 21:22, Andrew Cooper wrote:
>>>> Just as with c/s 96dc77b4b1 for XEN_SYSCTL_get_cpu_policy,
>>>> XEN_SYSCTL_get_cpu_featureset needs to become conditional.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Albeit I'd say "want", not "needs" in the description.
>> It occurs to me that XEN_SYSCTL_get_cpu_featureset is strictly a subset
>> of XEN_SYSCTL_get_cpu_policy, and that now I've adjusted the toolstack
>> onto get_cpu_policy, the sole remaining user is xen-cpuid.
>>
>> get_cpu_policy already has separate default and max indices, whereas
>> get_cpu_featureset was written before the need for this has become obvious.
>>
>> This leads to an asymmetry in xen-cpuid, where the -p (policy) option
>> provides two more sets of information than the featureset listing.
>>
>> Instead, I think I'd like to drop XEN_SYSCTL_get_cpu_featureset and
>> update the sole user to the more complete interface.
> Sounds like a good move to me.

Actually, after having spent almost a day trying to disentangle the
remains of this, I'm going to leave it for now.

It turns out it is still used by the legacy CPUID logic, and I could do
with getting a few other pieces of infrastructure before it is easy to
take out.

~Andrew

Patch
diff mbox series

diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index f55b67640a..36c17bf777 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -251,7 +251,7 @@  static void decode_featureset(const uint32_t *features,
     }
 }
 
-static void get_featureset(xc_interface *xch, unsigned int idx)
+static int get_featureset(xc_interface *xch, unsigned int idx)
 {
     struct fsinfo *f = &featuresets[idx];
 
@@ -261,8 +261,7 @@  static void get_featureset(xc_interface *xch, unsigned int idx)
     if ( !f->fs )
         err(1, "calloc(, featureset)");
 
-    if ( xc_get_cpu_featureset(xch, idx, &f->len, f->fs) )
-        err(1, "xc_get_featureset()");
+    return xc_get_cpu_featureset(xch, idx, &f->len, f->fs);
 }
 
 static void dump_info(xc_interface *xch, bool detail)
@@ -294,7 +293,17 @@  static void dump_info(xc_interface *xch, bool detail)
     printf("\nDynamic sets:\n");
     for ( i = 0; i < ARRAY_SIZE(featuresets); ++i )
     {
-        get_featureset(xch, i);
+        if ( get_featureset(xch, i) )
+        {
+            if ( errno == EOPNOTSUPP )
+            {
+                printf("%s featureset not supported by Xen\n",
+                       featuresets[i].name);
+                continue;
+            }
+
+            err(1, "xc_get_featureset()");
+        }
 
         decode_featureset(featuresets[i].fs, featuresets[i].len,
                           featuresets[i].name, detail);
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 59a384023b..7ea8c38797 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -363,11 +363,15 @@  long arch_do_sysctl(
 
     case XEN_SYSCTL_get_cpu_featureset:
     {
-        static const struct cpuid_policy *const policy_table[] = {
+        static const struct cpuid_policy *const policy_table[4] = {
             [XEN_SYSCTL_cpu_featureset_raw]  = &raw_cpuid_policy,
             [XEN_SYSCTL_cpu_featureset_host] = &host_cpuid_policy,
+#ifdef CONFIG_PV
             [XEN_SYSCTL_cpu_featureset_pv]   = &pv_max_cpuid_policy,
+#endif
+#ifdef CONFIG_HVM
             [XEN_SYSCTL_cpu_featureset_hvm]  = &hvm_max_cpuid_policy,
+#endif
         };
         const struct cpuid_policy *p = NULL;
         uint32_t featureset[FSCAPINTS];
@@ -389,12 +393,17 @@  long arch_do_sysctl(
 
         /* Look up requested featureset. */
         if ( sysctl->u.cpu_featureset.index < ARRAY_SIZE(policy_table) )
+        {
             p = policy_table[sysctl->u.cpu_featureset.index];
 
-        /* Bad featureset index? */
-        if ( !p )
-            ret = -EINVAL;
+            if ( !p )
+                ret = -EOPNOTSUPP;
+        }
         else
+            /* Bad featureset index? */
+            ret = -EINVAL;
+
+        if ( !ret )
             cpuid_policy_to_featureset(p, featureset);
 
         /* Copy the requested featureset into place. */
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 4dfba39ed8..3d72fab49f 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -796,6 +796,8 @@  struct xen_sysctl_cpu_levelling_caps {
  *  - Host: The values Xen is using, (after command line overrides, etc).
  *  -   PV: Maximum set of features which can be given to a PV guest.
  *  -  HVM: Maximum set of features which can be given to a HVM guest.
+ * May fail with -EOPNOTSUPP if querying for PV or HVM data when support is
+ * compiled out of Xen.
  */
 struct xen_sysctl_cpu_featureset {
 #define XEN_SYSCTL_cpu_featureset_raw      0