| Message ID | 20200226202221.6555-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Default vs Max policies | expand |
On 26.02.2020 21:22, Andrew Cooper wrote: > @@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void) > { > calculate_raw_policy(); > calculate_host_policy(); > - calculate_hvm_max_policy(); > - calculate_pv_max_policy(); > + > + if ( IS_ENABLED(CONFIG_PV) ) > + calculate_pv_max_policy(); > + > + if ( hvm_enabled ) Any chance of talking you into doing things more symmetrically, by either also using IS_ENABLED(CONFIG_HVM) here or ... > + calculate_hvm_max_policy(); > } > > int init_domain_msr_policy(struct domain *d) > { > - struct msr_policy *mp = > - xmemdup(is_pv_domain(d) ? &pv_max_msr_policy > - : &hvm_max_msr_policy); > + struct msr_policy *mp = is_pv_domain(d) > + ? (IS_ENABLED(CONFIG_PV) ? &pv_max_msr_policy : NULL) > + : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL); ... (imo preferably) hvm_enabled here? Either way Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 27/02/2020 08:07, Jan Beulich wrote: > On 26.02.2020 21:22, Andrew Cooper wrote: >> @@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void) >> { >> calculate_raw_policy(); >> calculate_host_policy(); >> - calculate_hvm_max_policy(); >> - calculate_pv_max_policy(); >> + >> + if ( IS_ENABLED(CONFIG_PV) ) >> + calculate_pv_max_policy(); >> + >> + if ( hvm_enabled ) > > Any chance of talking you into doing things more symmetrically, > by either also using IS_ENABLED(CONFIG_HVM) here or ... > >> + calculate_hvm_max_policy(); >> } >> >> int init_domain_msr_policy(struct domain *d) >> { >> - struct msr_policy *mp = >> - xmemdup(is_pv_domain(d) ? &pv_max_msr_policy >> - : &hvm_max_msr_policy); >> + struct msr_policy *mp = is_pv_domain(d) >> + ? (IS_ENABLED(CONFIG_PV) ? &pv_max_msr_policy : NULL) >> + : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL); > ... (imo preferably) hvm_enabled here? Either way > Reviewed-by: Jan Beulich <jbeulich@suse.com> The asymmetry is deliberate. In the former hunk, hvm_enabled is short-circuited to false for !CONFIG_HVM, and if I don't use hvm_enabled, here, then I've got to retain the logic at the top of calculate_hvm_max_policy(). That seems silly. In this later hunk, we are looking for the most efficient way to allow the compiler to discard the reference to hvm_max_msr_policy. Using hvm_enabled would be logically equivalent, but compile to more code in CONFIG_HVM case, as it is a real boolean needing checking. ~Andrew
On 27.02.2020 11:37, Andrew Cooper wrote: > On 27/02/2020 08:07, Jan Beulich wrote: >> On 26.02.2020 21:22, Andrew Cooper wrote: >>> @@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void) >>> { >>> calculate_raw_policy(); >>> calculate_host_policy(); >>> - calculate_hvm_max_policy(); >>> - calculate_pv_max_policy(); >>> + >>> + if ( IS_ENABLED(CONFIG_PV) ) >>> + calculate_pv_max_policy(); >>> + >>> + if ( hvm_enabled ) >> >> Any chance of talking you into doing things more symmetrically, >> by either also using IS_ENABLED(CONFIG_HVM) here or ... >> >>> + calculate_hvm_max_policy(); >>> } >>> >>> int init_domain_msr_policy(struct domain *d) >>> { >>> - struct msr_policy *mp = >>> - xmemdup(is_pv_domain(d) ? &pv_max_msr_policy >>> - : &hvm_max_msr_policy); >>> + struct msr_policy *mp = is_pv_domain(d) >>> + ? (IS_ENABLED(CONFIG_PV) ? &pv_max_msr_policy : NULL) >>> + : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL); >> ... (imo preferably) hvm_enabled here? Either way >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > The asymmetry is deliberate. > > In the former hunk, hvm_enabled is short-circuited to false for > !CONFIG_HVM, and if I don't use hvm_enabled, here, then I've got to > retain the logic at the top of calculate_hvm_max_policy(). That seems > silly. > > In this later hunk, we are looking for the most efficient way to allow > the compiler to discard the reference to hvm_max_msr_policy. Using > hvm_enabled would be logically equivalent, but compile to more code in > CONFIG_HVM case, as it is a real boolean needing checking. Fair enough, albeit I don't think the added evaluation of hvm_enabled would be the end of the world. Jan
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c index e39bb6dce4..738d7123f9 100644 --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -31,9 +31,13 @@ DEFINE_PER_CPU(uint32_t, tsc_aux); struct msr_policy __read_mostly raw_msr_policy, - __read_mostly host_msr_policy, - __read_mostly hvm_max_msr_policy, - __read_mostly pv_max_msr_policy; + __read_mostly host_msr_policy; +#ifdef CONFIG_PV +struct msr_policy __read_mostly pv_max_msr_policy; +#endif +#ifdef CONFIG_HVM +struct msr_policy __read_mostly hvm_max_msr_policy; +#endif static void __init calculate_raw_policy(void) { @@ -56,9 +60,6 @@ static void __init calculate_hvm_max_policy(void) { struct msr_policy *mp = &hvm_max_msr_policy; - if ( !hvm_enabled ) - return; - *mp = host_msr_policy; /* It's always possible to emulate CPUID faulting for HVM guests */ @@ -76,16 +77,27 @@ void __init init_guest_msr_policy(void) { calculate_raw_policy(); calculate_host_policy(); - calculate_hvm_max_policy(); - calculate_pv_max_policy(); + + if ( IS_ENABLED(CONFIG_PV) ) + calculate_pv_max_policy(); + + if ( hvm_enabled ) + calculate_hvm_max_policy(); } int init_domain_msr_policy(struct domain *d) { - struct msr_policy *mp = - xmemdup(is_pv_domain(d) ? &pv_max_msr_policy - : &hvm_max_msr_policy); + struct msr_policy *mp = is_pv_domain(d) + ? (IS_ENABLED(CONFIG_PV) ? &pv_max_msr_policy : NULL) + : (IS_ENABLED(CONFIG_HVM) ? &hvm_max_msr_policy : NULL); + + if ( !mp ) + { + ASSERT_UNREACHABLE(); + return -EOPNOTSUPP; + } + mp = xmemdup(mp); if ( !mp ) return -ENOMEM;
Arrange to compile out the PV or HVM logic and objects as applicable. This involves a bit of complexity in init_domain_msr_policy() as is_pv_domain() can't be evaulated at compile time. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/msr.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-)