Re: [PATCH v2 08/15] VMX: convert vmx_basic_msr

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/01/2024 2:39 pm, Jan Beulich wrote:
> On 18.12.2023 18:29, Andrew Cooper wrote:
>> On 27/11/2023 12:44 pm, Jan Beulich wrote:
>>> On 24.11.2023 23:41, Andrew Cooper wrote:
>>>> On 24/11/2023 8:41 am, Jan Beulich wrote:
>>>>> ... to a struct field, which is then going to be accompanied by other
>>>>> capability/control data presently living in individual variables. As
>>>>> this structure isn't supposed to be altered post-boot, put it in
>>>>> .data.ro_after_init right away.
>>>>>
>>>>> Suggested-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> For (usable) nested virt, we're going to need the VMX MSRs, in their
>>>> architectural form, in struct cpu_policy.  And just like CPUID features,
>>>> I want it to end up with nice bitfields to use.
>>>>
>>>> Looking through the rest of this series, vmx_caps ends up almost in
>>>> architectural form.
>>>>
>>>> Could I talk you into having a "struct vmx_msrs" (or similar - 'caps'
>>>> doesn't feel quite right here) in the policy object, and also
>>>> instantiating one instance of it for this purpose here?
>>> I was actually wondering while doing the conversion. The main reason I
>>> didn't go that route right away was that I wasn't really certain whether
>>> what I'd put there would the really be the (largely) final shape it
>>> wants to take there. (One thing you've likely noticed I didn't convert
>>> is _vmx_misc_cap, which right now only exists as a local variable in
>>> vmx_init_vmcs_config().)
>>>
>>>> AFAICT, it would only be a minor deviation to the latter half of this
>>>> series, but it would be an excellent start to fixing nested virt - and
>>>> getting this data in the policy really is the first task in getting the
>>>> ball rolling on nested virt.
>>> How much of a further change it would end up being (or where that change
>>> would occur) depends on another aspect: When put in cpu-policy.h (and I
>>> take it you mean the lib/ instance, not the asm/ one), it would seem
>>> natural and perhaps even necessary to properly introduce bitfields for
>>> each of the MSRs right away. That'll lead to a "raw" field as well. In
>>> VMX code (mostly its cpu_has_* #define-s), I'd then either need to use
>>> .raw (perhaps a little ugly here and there) or go with using the
>>> individual bitfields right away (likely eliminating the need for many of
>>> the constant #define-s), which increases the risk of inadvertent mistakes
>>> (and their overlooking during review).
>>>
>>>> I don't mind about serialising/de-serialsing it - that still has a bit
>>>> of userspace complexity to work out, and depends on some of the cleanup
>>>> still needing a repost.
>>>>
>>>> If you don't want to take the added space in cpu_policy yet, how about
>>>> having the declaration there and just forgo instantiating the subobject
>>>> in the short term?
>>> There's quite a bit of effectively dead space in the struct already; I
>>> think I wouldn't mind instantiating the struct there right away. So long
>>> as you're convinced it's going to be used there in not too distant a
>>> future.
>>>
>>> But: If I go as far, why would I introduce a global instance of the new
>>> struct? Wouldn't it then make more sense to use host_cpu_policy right
>>> away? I probably would keep populating it in vmx_init_vmcs_config() to
>>> limit churn for now, but consumers of the flags could then right away
>>> use the host policy.
>> George has stated an intent to pick nested virt up imminently.  I'll
>> have to defer to him on when this will actually start.
>>
>> But, sorting out this data in the policies is the next step, whenever
>> that occurs.
>>
>>
>> If you fancy going all the way to use the raw/host policy then great,
>> but I expect that would be a large amount of extra work, hence the
>> suggestion to just use the "inner" struct in the short term.
> Even the inner struct plan falls apart pretty quickly (or grows what
> needs doing by too much for my taste, in the context right here):
> While for basic_msr this works, and it would apparently also work
> for vmfunc and tertiary exec control (the latter is itself only part
> of a yet to be reviewed / approved patch), it doesn't for all the
> others with split 0-setting and 1-setting halves. This is because
> what VMX code wants are the calculated values to put in the VMCS,
> whereas imo in the policy we'd want to store both halves (and what
> exactly wants to be in the host policy there isn't really clear to
> me). As a result I can't create a single uniform structure properly
> serving both purposes. Nor could I have VMX code use the host
> policy for most of its capability checks.
>
> Thought / ideas?

If it's not actually trivial, then don't worry.

The policy does need to hold the architectural representation.  The
in-use settings need storing per-vCPU because they do (or need to me
made to) vary based on the configuration of the VM, and because they're
needed on every virtual vmentry when re-calculating VMCS02.

~Andrew