[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 00/13] lib{xc,xl}: support for guest MSR features
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |