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....@citrix.com> >>>>> Signed-off-by: Jan Beulich <jbeul...@suse.com> >>>> 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
