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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.02.2024 10:30, George Dunlap wrote:
> On Wed, Feb 21, 2024 at 6:52 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>>> 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.
> 
> So I don't know the history of how things got to be the way they are,
> nor really much about the code but what I've gathered from skimming
> through while creating this patch series.  But from that impression,
> the only issue I really see with the current code is the confusing
> naming.  The cpufeature.h code has this nice infrastructure to allow
> you to, for instance, enable or disable certain bits on the
> command-line; and the interface for querying all the different bits of
> functionality is all nicely put in one place.  Moving the
> svm_feature_flags into x86_capabilities would immediately allow SVM to
> take advantage of this infrastructure; it's not clear to me how this
> would be "needless".
> 
> Furthermore, it looks to me like host_cpu_policy is used as a starting
> point for generating pv_cpu_policy and hvm_cpu_policy, both of which
> are only used for guest cpuid generation.  Given that the format of
> those policies is fixed, and there's a lot of "copy this bit from the
> host policy wholesale", it seems like no matter what, you'd want a
> host_cpu_policy.
> 
> And in any case -- all that is kind of moot.  *Right now*,
> host_cpu_policy is only used for guest cpuid policy creation; *right
> now*, the nested virt features of AMD are handled in the
> host_cpu_policy; *right now*, we're advertising to guests bits which
> are not properly virtualized; *right now* these bits are actually set
> unconditionally, regardless of whether they're even available on the
> hardware; *right now*, Xen uses svm_feature_flags to determine its own
> use of TscRateMSR; so *right now*, removing this bit from
> host_cpu_policy won't prevent Xen from using TscRateMSR itself.
> 
> (Unless my understanding of the code is wrong, in which case I'd
> appreciate a correction.)

There's nothing wrong afaics, just missing at least one aspect: Did you
see all the featureset <-> policy conversions in cpu-policy.c? That (to
me at least) clearly is a sign of unnecessary duplication of the same
data. This goes as far as seeding the host policy from the raw one, just
to then immediately run x86_cpu_featureset_to_policy(), thus overwriting
a fair part of what was first taken from the raw policy. That's necessary
right now, because setup_{force,clear}_cpu_cap() act on
boot_cpu_data.x86_capability[], not the host policy.

As to the "needless" further up, it's only as far as moving those bits
into x86_capability[] would further duplicate information, rather than
(for that piece at least) putting them into the policies right away. But
yes, if the goal is to have setup_{force,clear}_cpu_cap() be able to
control those bits as well, then going the intermediate step would be
unavoidable at this point in time.

Jan