Re: [PATCH 00/13] lib{xc,xl}: support for guest MSR features

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/06/2023 2:10 pm, Roger Pau Monne wrote:
> Hello,
>
> The following series adds support for handling guest MSR features as
> defined in arch-x86/cpufeatureset.h.
>
> The end result is the user being able to use such features with the
> xl.cfg(5) cpuid option.  This also involves adding support to all the
> underlying layers, so both libxl and libxc also get new functionality in
> order to properly parse those.
>
> In order for such support to be as transparent as possible for existing
> users of libxl, both libxl_cpuid_policy_list and libxl_cpuid_policy are
> modified, so that the libxl_cpuid_policy_list type is no longer an array
> anymore, and libxl_cpuid_policy is converted into a structure with
> two fields to hold both a CPUID and MSR arrays.  It's my thinking that
> current users of libxl had no business in poking at
> libxl_cpuid_policy_list, since the underlying type (struct
> xc_xend_cpuid) is opaque in that context.  Also the format of the array
> (with a terminating empty element) is not documented in the public
> headers.
>
> Some of the patches had been salvaged from a previous series of mine to
> introduce better cpu_policy management support in libxc and libxl, and
> hence contain some version notes about changes, or are already reviewed.
>
> Thanks, Roger.
>
> Roger Pau Monne (13):
>   libs/guest: simplify xc_cpuid_apply_policy()
>   libx86: introduce helper to fetch cpuid leaf
>   libs/guest: allow fetching a specific CPUID leaf from a cpu policy
>   libx86: introduce helper to fetch msr entry
>   libs/guest: allow fetching a specific MSR entry from a cpu policy

I'm somewhat loath to introduce wrappers like this to structures which
intentionally have O(1) lookup by index.

These only exist (AFAICT) to let libxl poke inside an opaque object, but
I don't see them used.  Furthermore, I'm not sure that extending
test-cpu-policy is much use - the existing {,de}serialise tests already
exercise the logic, as you factored it out of said logic.

>   libs/guest: replace usage of host featureset in xc_cpuid_apply_policy()

This looks fine, but it's fairly dependent on patch 1 which needs a
rearrangement.

>   libs/guest: rework xc_cpuid_xend_policy

Here, the xend is about the format of the argument, so is important to
stay as part of the name.

Furthermore, this helper is deliberately not exported from
xg_cpuid_x86.c such that xc_cpuid_apply_policy() is the single interface
to use, and AFAICT that's still the case after the series too.

>   libs/guest: introduce support for setting guest MSRs
>   libxl: change the type of libxl_cpuid_policy_list
>   libxl: introduce MSR data in libxl_cpuid_policy
>   libxl: split logic to parse user provided CPUID features
>   libxl: use the cpuid feature names from cpufeatureset.h
>   libxl: add support for parsing MSR features

So by the end here, we're still using the cpuid="host:..." format, using
enumerations from cpufeatureset.h, with a brand new MSR type, and a
hand-coded mapping between a featureset number and a register?

I think this would be far more simple it it just used the featureset
bitmap which already exists in xc_cpuid_apply_policy(), rather than
borrowing the featureset names and coding around an interface that
already exists.

I'm wary about the names themselves.  I already dislike that
cpufeatureset is in public/ but this is exposing it more than
previously.  We have previously elected to tweak naming in
cpufeatureset, and there's currently a mess with feature naming where
Intel and AMD have the same named bit in different positions.  IMO we
need to retain this flexibility - perhaps we can do that by just
extending libxl's alias lists if we decide we need to change the names
in Xen.

~Andrew