Re: [Xen-devel] [PATCH v2 8/8] x86/cpuid: Enable CPUID Faulting for the control domain by default

[Jan Beulich <jbeulich@suse.com>]

On 12.09.2019 20:55, Andrew Cooper wrote:
> The domain builder no longer uses local CPUID instructions for policy
> decisions.  This resolves a key issue for PVH dom0's.  However, as PV dom0's
> have never had faulting enforced, leave a command line option to restore the
> old behaviour.
> 
> In ctxt_switch_levelling(), invert the first cpu_has_cpuid_faulting condition
> to reduce the indentation for the CPUID faulting logic.
> 
> Advertise virtualised faulting support to control domains unless the opt-out
> has been used.
> 
> 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>
> 
> opt_dom0_cpuid_faulting would ideally live in dom0_build.c next to
> opt_dom0_verbose, but the file is currently .init

And it should remain so imo.

> v2:
>  * Introduce a command line option to retain old behaviour.
>  * Advertise virtualised faulting support to dom0 when it is used.
> 
> RFC: The previous logic was slightly buggy in that even PVH dom0's had
> virtualised faulting support hidden from them.  Given that this case always
> hits the CPUID intercept, how much do we care about retaining the old
> behaviour?

I'm having trouble with this statement: Neither the description nor
the actual code change suggest you alter behavior in this regard
(i.e. with the option used PVH would still be treated the same as
PV afaict). Yet here you seem to suggest things change with this
patch.

As to the question, I think I'd consider this a bugfix, and hence
for the behavior to be okay to change. But as per above I would
expect the change to init_domain_msr_policy() to also include
adding is_pv_domain() to the conditional, or alternatively to
override opt_dom0_cpuid_faulting to true if a PVH Dom0 is being
built.

> ---
>  xen/arch/x86/cpu/common.c   | 59 +++++++++++++++++++++++----------------------
>  xen/arch/x86/dom0_build.c   |  2 ++
>  xen/arch/x86/msr.c          |  3 ++-
>  xen/include/asm-x86/setup.h |  1 +
>  4 files changed, 35 insertions(+), 30 deletions(-)

Please also update the command line doc.

> @@ -92,7 +93,7 @@ int init_domain_msr_policy(struct domain *d)
>          return -ENOMEM;
>  
>      /* See comment in intel_ctxt_switch_levelling() */
> -    if ( is_control_domain(d) )
> +    if ( !opt_dom0_cpuid_faulting && is_control_domain(d) )
>          mp->platform_info.cpuid_faulting = false;

While unrelated to the overall change here, I think the comment
would better be updated too, to drop the intel_ prefix of the
function name.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel