Re: [PATCH 4/6] nestedsvm: Disable TscRateMSR

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.02.2024 09:48, George Dunlap wrote:
> On Mon, Feb 19, 2024 at 11:22 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 06.02.2024 02:20, George Dunlap wrote:
>>> For now, just disable the functionality entirely until we can
>>> implement it properly:
>>>
>>> - Don't set TSCRATEMSR in the host CPUID policy
>>
>> This goes too far: This way you would (in principle) also affect guests
>> with nesting disabled. According to the earlier parts of the description
>> there's also no issue with it in that case. What you want to make sure
>> it that in the HVM policy the bit isn't set.
>>
>> While presently resolving to cpu_has_svm_feature(), I think
>> cpu_has_tsc_ratio really ought to resolve to the host policy field.
>> Of course then requiring the host policy to reflect reality rather than
>> having what is "always emulated". IOW ...
>>
>>> --- a/xen/arch/x86/cpu-policy.c
>>> +++ b/xen/arch/x86/cpu-policy.c
>>> @@ -407,8 +407,7 @@ static void __init calculate_host_policy(void)
>>>                                 (1u << SVM_FEATURE_PAUSEFILTER) |
>>>                                 (1u << SVM_FEATURE_DECODEASSISTS));
>>>          /* Enable features which are always emulated. */
>>> -        p->extd.raw[0xa].d |= ((1u << SVM_FEATURE_VMCBCLEAN) |
>>> -                               (1u << SVM_FEATURE_TSCRATEMSR));
>>> +        p->extd.raw[0xa].d |= (1u << SVM_FEATURE_VMCBCLEAN);
>>
>> ... this likely wants replacing altogether by not overriding what we
>> found in hardware, which would apparently mean moving the two bit
>> masks to the earlier "clamping" expression.
>>
>> But then of course Andrew may know of reasons why all of this is done
>> in calculate_host_policy() in the first place, rather than in HVM
>> policy calculation.
> 
> It sounds like maybe you're confusing host_policy with
> x86_capabilities?  From what I can tell:
> 
> *  the "basic" cpu_has_X macros resolve to boot_cpu_has(), which
> resolves to cpu_has(&boot_cpu_data, ...), which is completely
> independent of the cpu-policy.c:host_cpu_policy
> 
> * cpu-policy.c:host_cpu_policy only affects what is advertised to
> guests, via {pv,hvm}_cpu_policy and featureset bits.  Most notably a
> quick skim doesn't show any mechanism by which host_cpu_policy could
> affect what features Xen itself decides to use.

I'm not mixing the two, no; the two are still insufficiently disentangled.
There's really no reason (long term) to have both host policy and
x86_capabilities. Therefore I'd prefer if new code (including a basically
fundamental re-write as is going to be needed for nested) to avoid
needlessly further extending x86_capabilities. Unless of course there's
something fundamentally wrong with eliminating the redundancy, which
likely Andrew would be in the best position to point out.

> Not sure exactly why the nested virt stuff is done at the
> host_cpu_policy level rather than the hvm_cpu_policy level, but since
> that's where it is, that's where we need to change it.
> 
> FWIW, as I said in response to your comment on 2/6, it would be nicer
> if we moved svm_feature_flags into the "capabilities" section; but
> that's a different set of work.

Can as well reply here then, rather than there: If the movement from
host to HVM policy was done first, the patch here could more sanely go
on top, and patch 2 could then also go on top, converting the separate
variable to host policy accesses, quite possibly introducing a similar
wrapper as you introduce there right now.

But no, I'm not meaning to make this a requirement; this would merely be
an imo better approach. My ack there stands, in case you want to keep
(and commit) the change as is.

Jan