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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan