| Message ID | d07fb86f-ef9c-4c2b-ba9a-f1e5c813d45d@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/HVM: misc tidying | expand |
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@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@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? 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. 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? ~Andrew
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@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@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. Jan
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@citrix.com> >>> Signed-off-by: Jan Beulich <jbeulich@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. 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
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@citrix.com> >>>> Signed-off-by: Jan Beulich <jbeulich@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? 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
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@citrix.com> >>>>> Signed-off-by: Jan Beulich <jbeulich@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
On 09.01.2024 15:54, Andrew Cooper wrote: > On 09/01/2024 2:39 pm, Jan Beulich wrote: >> 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. Would it help if I did a hybrid approach, i.e. move to raw/host policies only what can be easily moved, with the rest kept as is (perhaps with vmx_caps renamed to vmx_ctls then)? Jan
--- a/xen/arch/x86/hvm/vmx/vmcs.c +++ b/xen/arch/x86/hvm/vmx/vmcs.c @@ -161,6 +161,7 @@ static int cf_check parse_ept_param_runt #endif /* Dynamic (run-time adjusted) execution control flags. */ +struct vmx_caps __ro_after_init vmx_caps; u32 vmx_pin_based_exec_control __read_mostly; u32 vmx_cpu_based_exec_control __read_mostly; u32 vmx_secondary_exec_control __read_mostly; @@ -174,8 +175,7 @@ static DEFINE_PER_CPU(paddr_t, current_v static DEFINE_PER_CPU(struct list_head, active_vmcs_list); DEFINE_PER_CPU(bool, vmxon); -#define vmcs_revision_id (vmx_basic_msr & VMX_BASIC_REVISION_MASK) -u64 __read_mostly vmx_basic_msr; +#define vmcs_revision_id (vmx_caps.basic_msr & VMX_BASIC_REVISION_MASK) static void __init vmx_display_features(void) { @@ -470,8 +470,8 @@ static int vmx_init_vmcs_config(bool bsp vmx_ept_vpid_cap = _vmx_ept_vpid_cap; vmx_vmexit_control = _vmx_vmexit_control; vmx_vmentry_control = _vmx_vmentry_control; - vmx_basic_msr = ((u64)vmx_basic_msr_high << 32) | - vmx_basic_msr_low; + vmx_caps.basic_msr = ((uint64_t)vmx_basic_msr_high << 32) | + vmx_basic_msr_low; vmx_vmfunc = _vmx_vmfunc; vmx_display_features(); @@ -522,7 +522,7 @@ static int vmx_init_vmcs_config(bool bsp mismatch = 1; } if ( (vmx_basic_msr_high & (VMX_BASIC_VMCS_SIZE_MASK >> 32)) != - ((vmx_basic_msr & VMX_BASIC_VMCS_SIZE_MASK) >> 32) ) + ((vmx_caps.basic_msr & VMX_BASIC_VMCS_SIZE_MASK) >> 32) ) { printk("VMX: CPU%d unexpected VMCS size %Lu\n", smp_processor_id(), @@ -2169,7 +2169,7 @@ int __init vmx_vmcs_init(void) * _vmx_vcpu_up() may have made it past feature identification. * Make sure all dependent features are off as well. */ - vmx_basic_msr = 0; + memset(&vmx_caps, 0, sizeof(vmx_caps)); vmx_pin_based_exec_control = 0; vmx_cpu_based_exec_control = 0; vmx_secondary_exec_control = 0; --- a/xen/arch/x86/include/asm/hvm/vmx/vmcs.h +++ b/xen/arch/x86/include/asm/hvm/vmx/vmcs.h @@ -283,6 +283,12 @@ extern u64 vmx_ept_vpid_cap; #define VMX_TSC_MULTIPLIER_MAX 0xffffffffffffffffULL +/* Capabilities and dynamic (run-time adjusted) execution control flags. */ +struct vmx_caps { + uint64_t basic_msr; +}; +extern struct vmx_caps vmx_caps; + #define cpu_has_wbinvd_exiting \ (vmx_secondary_exec_control & SECONDARY_EXEC_WBINVD_EXITING) #define cpu_has_vmx_virtualize_apic_accesses \ @@ -366,9 +372,8 @@ extern u64 vmx_ept_vpid_cap; */ #define VMX_BASIC_DEFAULT1_ZERO (1ULL << 55) -extern u64 vmx_basic_msr; #define cpu_has_vmx_ins_outs_instr_info \ - (!!(vmx_basic_msr & VMX_BASIC_INS_OUT_INFO)) + (!!(vmx_caps.basic_msr & VMX_BASIC_INS_OUT_INFO)) /* Guest interrupt status */ #define VMX_GUEST_INTR_STATUS_SUBFIELD_BITMASK 0x0FF --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1556,7 +1556,7 @@ static int nvmx_handle_vmxon(struct cpu_ rc = hvm_copy_from_guest_phys(&nvmcs_revid, gpa, sizeof(nvmcs_revid)); if ( rc != HVMTRANS_okay || (nvmcs_revid & ~VMX_BASIC_REVISION_MASK) || - ((nvmcs_revid ^ vmx_basic_msr) & VMX_BASIC_REVISION_MASK) ) + ((nvmcs_revid ^ vmx_caps.basic_msr) & VMX_BASIC_REVISION_MASK) ) { vmfail_invalid(regs); return X86EMUL_OKAY; @@ -1794,7 +1794,7 @@ static int nvmx_handle_vmptrld(struct cp { struct vmcs_struct *vvmcs = vvmcx; - if ( ((vvmcs->revision_id ^ vmx_basic_msr) & + if ( ((vvmcs->revision_id ^ vmx_caps.basic_msr) & VMX_BASIC_REVISION_MASK) || (!cpu_has_vmx_vmcs_shadowing && (vvmcs->revision_id & ~VMX_BASIC_REVISION_MASK)) ) @@ -2187,7 +2187,7 @@ int nvmx_msr_read_intercept(unsigned int case MSR_IA32_VMX_TRUE_PROCBASED_CTLS: case MSR_IA32_VMX_TRUE_EXIT_CTLS: case MSR_IA32_VMX_TRUE_ENTRY_CTLS: - if ( !(vmx_basic_msr & VMX_BASIC_DEFAULT1_ZERO) ) + if ( !(vmx_caps.basic_msr & VMX_BASIC_DEFAULT1_ZERO) ) return 0; break;
... 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@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- v2: New.