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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

Jan

> Conversion to bitfields would want to be separate patch anyway, at which
> point an A/B compile can confirm whether there was no resulting change.
> 
> I'm happy if you want to do all of this, but it's a lot of work, and
> simply having the data in plain architectural uint64_t in the host
> policy is something that I thought would be a very minor change to your
> current series, but with a useful step towards nested virt.
> 
> One open question, before we get too far into this, is still whether to
> express half of these as MSR-features like ARCH_CAPS.  Linux does, and
> there is a very complex set of dependencies between certain properties,
> although I have a sneaking suspicion that the dependency logic will
> needed at runtime as the L1 hypervisor changes the various controls.
> 
> ~Andrew