Introduce support for handling MSR features in
libxl_cpuid_parse_config().  The MSR policies are added to the
libxl_cpuid_policy like the CPUID one, which gets passed to
xc_cpuid_apply_policy().

This allows existing users of libxl to provide MSR related features as
key=value pairs to libxl_cpuid_parse_config() without requiring the
usage of a different API.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libs/light/libxl_cpuid.c | 61 +++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index b1c4f8f2f45b..86a08f29a19c 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -158,6 +158,57 @@ static int cpuid_add(libxl_cpuid_policy_list *policy,
     return 0;
 }
 
+static struct xc_msr *msr_find_match(libxl_cpuid_policy_list *pl, uint32_t index)
+{
+    unsigned int i = 0;
+    libxl_cpuid_policy_list policy = *pl;
+
+    if (policy == NULL)
+        policy = *pl = calloc(1, sizeof(*policy));
+
+    if (policy->msr != NULL)
+        for (i = 0; policy->msr[i].index != XC_MSR_INPUT_UNUSED; i++)
+            if (policy->msr[i].index == index)
+                return &policy->msr[i];
+
+    policy->msr = realloc(policy->msr, sizeof(struct xc_msr) * (i + 2));
+    policy->msr[i].index = index;
+    memset(policy->msr[i].policy, 'x', ARRAY_SIZE(policy->msr[0].policy) - 1);
+    policy->msr[i].policy[ARRAY_SIZE(policy->msr[0].policy) - 1] = '\0';
+    policy->msr[i + 1].index = XC_MSR_INPUT_UNUSED;
+
+    return &policy->msr[i];
+}
+
+static int msr_add(libxl_cpuid_policy_list *policy, uint32_t index, unsigned int bit,
+                   const char *val)
+{
+    struct xc_msr *entry = msr_find_match(policy, index);
+
+    /* Only allow options taking a character for MSRs, no values allowed. */
+    if (strlen(val) != 1)
+        return 3;
+
+    switch (val[0]) {
+    case '0':
+    case '1':
+    case 'x':
+    case 'k':
+        entry->policy[63 - bit] = val[0];
+        break;
+
+    case 's':
+        /* Translate s -> k as xc_msr doesn't support the deprecated 's'. */
+        entry->policy[63 - bit] = 'k';
+        break;
+
+    default:
+        return 3;
+    }
+
+    return 0;
+}
+
 struct feature_name {
     const char *name;
     unsigned int bit;
@@ -337,7 +388,15 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *policy, const char* str)
     }
 
     case FEAT_MSR:
-        return 2;
+    {
+        unsigned int bit = feat->bit % 32;
+
+        if (feature_to_policy[feat->bit / 32].msr.reg == CPUID_REG_EDX)
+            bit += 32;
+
+        return msr_add(policy, feature_to_policy[feat->bit / 32].msr.index,
+                       bit, val);
+    }
     }
 
     return 2;
-- 
2.41.0

On Tue, Jul 11, 2023 at 11:22:30AM +0200, Roger Pau Monne wrote:
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index b1c4f8f2f45b..86a08f29a19c 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -158,6 +158,57 @@ static int cpuid_add(libxl_cpuid_policy_list *policy,
>      return 0;
>  }
>  
> +static struct xc_msr *msr_find_match(libxl_cpuid_policy_list *pl, uint32_t index)
> +{
> +    unsigned int i = 0;
> +    libxl_cpuid_policy_list policy = *pl;
> +
> +    if (policy == NULL)
> +        policy = *pl = calloc(1, sizeof(*policy));
> +
> +    if (policy->msr != NULL)
> +        for (i = 0; policy->msr[i].index != XC_MSR_INPUT_UNUSED; i++)

Could you add { } for this two blocks? One line after a if() without { }
is ok, but not more.

> +            if (policy->msr[i].index == index)
> +                return &policy->msr[i];
> +
> +    policy->msr = realloc(policy->msr, sizeof(struct xc_msr) * (i + 2));
> +    policy->msr[i].index = index;
> +    memset(policy->msr[i].policy, 'x', ARRAY_SIZE(policy->msr[0].policy) - 1);

Is this "array_size() - 1" correct? The -1 need to go, right?

> +    policy->msr[i].policy[ARRAY_SIZE(policy->msr[0].policy) - 1] = '\0';

Is it for convenience? Maybe for easier debugging (printf)? Also, I
guess having a NUL at the end mean the -1 on the previous statement kind
of useful.

> +    policy->msr[i + 1].index = XC_MSR_INPUT_UNUSED;
> +
> +    return &policy->msr[i];
> +}

Thanks,

-- 
Anthony PERARD

On Thu, Jul 13, 2023 at 11:39:53AM +0100, Anthony PERARD wrote:
> On Tue, Jul 11, 2023 at 11:22:30AM +0200, Roger Pau Monne wrote:
> > diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> > index b1c4f8f2f45b..86a08f29a19c 100644
> > --- a/tools/libs/light/libxl_cpuid.c
> > +++ b/tools/libs/light/libxl_cpuid.c
> > @@ -158,6 +158,57 @@ static int cpuid_add(libxl_cpuid_policy_list *policy,
> >      return 0;
> >  }
> >  
> > +static struct xc_msr *msr_find_match(libxl_cpuid_policy_list *pl, uint32_t index)
> > +{
> > +    unsigned int i = 0;
> > +    libxl_cpuid_policy_list policy = *pl;
> > +
> > +    if (policy == NULL)
> > +        policy = *pl = calloc(1, sizeof(*policy));
> > +
> > +    if (policy->msr != NULL)
> > +        for (i = 0; policy->msr[i].index != XC_MSR_INPUT_UNUSED; i++)
> 
> Could you add { } for this two blocks? One line after a if() without { }
> is ok, but not more.

Sure.

> > +            if (policy->msr[i].index == index)
> > +                return &policy->msr[i];
> > +
> > +    policy->msr = realloc(policy->msr, sizeof(struct xc_msr) * (i + 2));
> > +    policy->msr[i].index = index;
> > +    memset(policy->msr[i].policy, 'x', ARRAY_SIZE(policy->msr[0].policy) - 1);
> 
> Is this "array_size() - 1" correct? The -1 need to go, right?
> 
> > +    policy->msr[i].policy[ARRAY_SIZE(policy->msr[0].policy) - 1] = '\0';
> 
> Is it for convenience? Maybe for easier debugging (printf)? Also, I
> guess having a NUL at the end mean the -1 on the previous statement kind
> of useful.

Yes, it's also to match the format of the policy string used by
xc_xend_cpuid, which also has a terminating zero.

Are you OK with this?

Thanks, Roger.

On Mon, Jul 17, 2023 at 04:46:25PM +0200, Roger Pau Monné wrote:
> On Thu, Jul 13, 2023 at 11:39:53AM +0100, Anthony PERARD wrote:
> > On Tue, Jul 11, 2023 at 11:22:30AM +0200, Roger Pau Monne wrote:
> > > diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> > > index b1c4f8f2f45b..86a08f29a19c 100644
> > > --- a/tools/libs/light/libxl_cpuid.c
> > > +++ b/tools/libs/light/libxl_cpuid.c
> > > @@ -158,6 +158,57 @@ static int cpuid_add(libxl_cpuid_policy_list *policy,
> > >      return 0;
> > >  }
> > >  
> > > +static struct xc_msr *msr_find_match(libxl_cpuid_policy_list *pl, uint32_t index)
> > > +{
> > > +    unsigned int i = 0;
> > > +    libxl_cpuid_policy_list policy = *pl;
> > > +
> > > +    if (policy == NULL)
> > > +        policy = *pl = calloc(1, sizeof(*policy));
> > > +
> > > +    if (policy->msr != NULL)
> > > +        for (i = 0; policy->msr[i].index != XC_MSR_INPUT_UNUSED; i++)
> > 
> > Could you add { } for this two blocks? One line after a if() without { }
> > is ok, but not more.
> 
> Sure.
> 
> > > +            if (policy->msr[i].index == index)
> > > +                return &policy->msr[i];
> > > +
> > > +    policy->msr = realloc(policy->msr, sizeof(struct xc_msr) * (i + 2));
> > > +    policy->msr[i].index = index;
> > > +    memset(policy->msr[i].policy, 'x', ARRAY_SIZE(policy->msr[0].policy) - 1);
> > 
> > Is this "array_size() - 1" correct? The -1 need to go, right?
> > 
> > > +    policy->msr[i].policy[ARRAY_SIZE(policy->msr[0].policy) - 1] = '\0';
> > 
> > Is it for convenience? Maybe for easier debugging (printf)? Also, I
> > guess having a NUL at the end mean the -1 on the previous statement kind
> > of useful.
> 
> Yes, it's also to match the format of the policy string used by
> xc_xend_cpuid, which also has a terminating zero.
> 
> Are you OK with this?

Yes.

With the other style change done:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD