Re: [PATCH 1/4] x86: suppress XPTI-related TLB flushes when possible

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 13/03/2019 12:38, Jan Beulich wrote:
> When there's no XPTI-enabled PV domain at all, there's no need to issue
> respective TLB flushes. Hardwire opt_xpti_* to false when !PV, and
> record the creation of PV domains by bumping opt_xpti_* accordingly.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: The hardwiring to false could be extended to opt_pv_l1tf_* and (for
>      !HVM) opt_l1d_flush as well.

For what purpose?  opt_pv_l1tf_* is only read inside a CONFIG_PV section
(despite how pv_l1tf_domain_init() is laid out - there is an outer ifdef
as well), while opt_l1d_flush influences the contents of the guests MSR
load list, which is inherently VT-x only.

> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -270,6 +270,9 @@ void pv_domain_destroy(struct domain *d)
>      destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
>                                GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
>  
> +    opt_xpti_hwdom -= IS_ENABLED(CONFIG_LATE_HWDOM) &&
> +                      !d->domain_id && opt_xpti_hwdom;
> +
>      XFREE(d->arch.pv.cpuidmasks);
>  
>      FREE_XENHEAP_PAGE(d->arch.pv.gdt_ldt_l1tab);
> @@ -308,7 +311,16 @@ int pv_domain_initialise(struct domain *
>      /* 64-bit PV guest by default. */
>      d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>  
> -    d->arch.pv.xpti = is_hardware_domain(d) ? opt_xpti_hwdom : opt_xpti_domu;
> +    if ( is_hardware_domain(d) && opt_xpti_hwdom )
> +    {
> +        d->arch.pv.xpti = true;
> +        ++opt_xpti_hwdom;
> +    }
> +    if ( !is_hardware_domain(d) && opt_xpti_domu )
> +    {
> +        d->arch.pv.xpti = true;
> +        opt_xpti_domu = 2;

This logic is asymetric.  We will retain TLB flushing after the final
domu has shut down.

I'm also not sure about the hwdom logic.  There is guaranteed to be
exactly one, and Xen will shut down when it goes offline, but it may not
be a PV guest.  opt_xpti_hwdom should be unconditionally 2 on this path
(I think).

> --- a/xen/include/asm-x86/spec_ctrl.h
> +++ b/xen/include/asm-x86/spec_ctrl.h
> @@ -42,7 +42,12 @@ extern bool bsp_delay_spec_ctrl;
>  extern uint8_t default_xen_spec_ctrl;
>  extern uint8_t default_spec_ctrl_flags;
>  
> +#ifdef CONFIG_PV
>  extern int8_t opt_xpti_hwdom, opt_xpti_domu;
> +#else
> +# define opt_xpti_hwdom false
> +# define opt_xpti_domu false
> +#endif

These now have more complicated interaction with flushing.  At the
absolute minimum, it needs a sentence or two about the new semantics.

~Andrew

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