On 01/05/2024 11:00 am, George Dunlap wrote:
> On Mon, Apr 29, 2024 at 4:16 PM Andrew Cooper <andrew.coop...@citrix.com>
> wrote:
>> These will replace svm_feature_flags and the SVM_FEATURE_* constants over the
>> next few changes. Take the opportunity to rationalise some names.
>>
>> Drop the opencoded "inherit from host" logic in calculate_hvm_max_policy()
>> and
>> use 'h'/'!' annotations. The logic needs to operate on fs, not the policy
>> object, given its position within the function.
>>
>> Drop some trailing whitespace introduced when this block of code was last
>> moved.
>>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@citrix.com>
>> CC: Stefano Stabellini <sstabell...@kernel.org>
>> CC: Xenia Ragiadakou <xenia.ragiada...@amd.com>
>> CC: Sergiy Kibrik <sergiy_kib...@epam.com>
>> CC: George Dunlap <george.dun...@citrix.com>
>> CC: Andrei Semenov <andrei.seme...@vates.fr>
>> CC: Vaishali Thakkar <vaishali.thak...@vates.tech>
>> ---
>> tools/misc/xen-cpuid.c | 11 +++++++++++
>> xen/arch/x86/cpu-policy.c | 17 +++++------------
>> xen/include/public/arch-x86/cpufeatureset.h | 14 ++++++++++++++
>> xen/tools/gen-cpuid.py | 3 +++
>> 4 files changed, 33 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
>> index ab09410a05d6..0d01b0e797f1 100644
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -266,6 +266,17 @@ static const char *const str_m10Ah[32] =
>>
>> static const char *const str_eAd[32] =
>> {
>> + [ 0] = "npt", [ 1] = "v-lbr",
>> + [ 2] = "svm-lock", [ 3] = "nrips",
>> + [ 4] = "v-tsc-rate", [ 5] = "vmcb-cleanbits",
>> + [ 6] = "flush-by-asid", [ 7] = "decode-assist",
>> +
>> + [10] = "pause-filter",
>> + [12] = "pause-thresh",
>> + /* 14 */ [15] = "v-loadsave",
>> + [16] = "v-gif",
>> + /* 18 */ [19] = "npt-sss",
>> + [20] = "v-spec-ctrl",
>> };
>>
>> static const char *const str_e1Fa[32] =
>> diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
>> index 4b6d96276399..da4401047e89 100644
>> --- a/xen/arch/x86/cpu-policy.c
>> +++ b/xen/arch/x86/cpu-policy.c
>> @@ -9,7 +9,6 @@
>> #include <asm/amd.h>
>> #include <asm/cpu-policy.h>
>> #include <asm/hvm/nestedhvm.h>
>> -#include <asm/hvm/svm/svm.h>
>> #include <asm/intel-family.h>
>> #include <asm/msr-index.h>
>> #include <asm/paging.h>
>> @@ -748,22 +747,16 @@ static void __init calculate_hvm_max_policy(void)
>> if ( !cpu_has_vmx )
>> __clear_bit(X86_FEATURE_PKS, fs);
>>
>> - /*
>> + /*
>> * Make adjustments to possible (nested) virtualization features exposed
>> * to the guest
>> */
>> - if ( p->extd.svm )
>> + if ( test_bit(X86_FEATURE_SVM, fs) )
>> {
>> - /* Clamp to implemented features which require hardware support. */
>> - p->extd.raw[0xa].d &= ((1u << SVM_FEATURE_NPT) |
>> - (1u << SVM_FEATURE_LBRV) |
>> - (1u << SVM_FEATURE_NRIPS) |
>> - (1u << SVM_FEATURE_PAUSEFILTER) |
>> - (1u << SVM_FEATURE_DECODEASSISTS));
>> - /* Enable features which are always emulated. */
>> - p->extd.raw[0xa].d |= (1u << SVM_FEATURE_VMCBCLEAN);
>> + /* Xen always emulates cleanbits. */
>> + __set_bit(X86_FEATURE_VMCB_CLEANBITS, fs);
>> }
> What about this line, at the end of recalculate_cpuid_policy()?
>
> if ( !p->extd.svm )
> p->extd.raw[0xa] = EMPTY_LEAF;
>
> Is there a reason to continue to operate directly on the policy here,
> or would it be better to do this up in the featureset section?
That's still needed.
Otherwise in a !SVM VM you still see svm_rev and nr_asids in a leaf that
should be all zeroes.
~Andrew
