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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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