Re: [Xen-devel] [PATCH v8 3/9] xen/x86: support per-domain flag for xpti

[Jan Beulich]

>>> On 18.04.18 at 17:54,  wrote:
> On 18/04/18 17:45, Jan Beulich wrote:
> On 18.04.18 at 17:33,  wrote:
>>> On 18/04/18 17:29, Jan Beulich wrote:
>>> On 18.04.18 at 10:30,  wrote:
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -967,7 +967,7 @@ static int shadow_set_l4e(struct domain *d,
>  sh_put_ref(d, osl3mfn, paddr);
>  }
>  
> -if ( !cpu_has_no_xpti )
> +if ( is_pv_domain(d) && d->arch.pv_domain.xpti )
>  /*
>   * Lazy flushing is enough: either we do a TLB flush right 
> afterwards
>   * which will pick up the new root page table on all affected 
> cpus

 How come the is_pv_domain() is appearing only here?
>>>
>>> It is mandatory for testing the per-domain xpti flag. I could add it in
>>> patch 1 already if you like that better.
>> 
>> Well, if you added it earlier, some unnecessary IPIs would be suppressed
>> right away.
> 
> Which IPIs? There is no IPI involved here. We are just setting the flags
> from the current cpu for all cpus which need to picḱ it up. That's the
> reason for the comment regarding "lazy flushing".

Oh, I did assume flush_root_pgt_mask() would be a function that I had
seen in prior versions, invoking the processing of FLUSH_ROOT_PGTBL on
remote CPUs.

Clearly with the introduction of a new function you should have dropped
my R-b; I'll comment there.

Jan


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

Re: [Xen-devel] [PATCH v8 3/9] xen/x86: support per-domain flag for xpti

[Jan Beulich]

>>> On 18.04.18 at 17:33,  wrote:
> On 18/04/18 17:29, Jan Beulich wrote:
> On 18.04.18 at 10:30,  wrote:
>>> --- a/xen/arch/x86/mm/shadow/multi.c
>>> +++ b/xen/arch/x86/mm/shadow/multi.c
>>> @@ -967,7 +967,7 @@ static int shadow_set_l4e(struct domain *d,
>>>  sh_put_ref(d, osl3mfn, paddr);
>>>  }
>>>  
>>> -if ( !cpu_has_no_xpti )
>>> +if ( is_pv_domain(d) && d->arch.pv_domain.xpti )
>>>  /*
>>>   * Lazy flushing is enough: either we do a TLB flush right 
>>> afterwards
>>>   * which will pick up the new root page table on all affected cpus
>> 
>> How come the is_pv_domain() is appearing only here?
> 
> It is mandatory for testing the per-domain xpti flag. I could add it in
> patch 1 already if you like that better.

Well, if you added it earlier, some unnecessary IPIs would be suppressed
right away.

Jan



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

Re: [Xen-devel] [PATCH v8 3/9] xen/x86: support per-domain flag for xpti

[Juergen Gross]

On 18/04/18 11:42, Sergey Dyasli wrote:
> Hi Juergen,
> 
> 2 small requests from me below.
> 
> On Wed, 2018-04-18 at 10:30 +0200, Juergen Gross wrote:
>> Instead of switching XPTI globally on or off add a per-domain flag for
>> that purpose. This allows to modify the xpti boot parameter to support
>> running dom0 without Meltdown mitigations. Using "xpti=nodom0" as boot
>> parameter will achieve that.
>>
>> Move the xpti boot parameter handling to xen/arch/x86/pv/domain.c as
>> it is pv-domain specific.
>>
>> Signed-off-by: Juergen Gross 
>> Reviewed-by: Jan Beulich 
> 
> 
>> diff --git a/docs/misc/xen-command-line.markdown 
>> b/docs/misc/xen-command-line.markdown
>> index b353352adf..d4f758487a 100644
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1955,14 +1955,24 @@ clustered mode.  The default, given no hint from the 
>> **FADT**, is cluster
>>  mode.
>>  
>>  ### xpti
>> -> `= `
>> +> `= List of [ default |  | dom0= | domu= ]`
>>  
>> -> Default: `false` on AMD hardware
>> +> Default: `false` on hardware not vulnerable to Meltdown (e.g. AMD)
> 
> Could this line please be changed to:
> 
> `false` on hardware known not to be vulnerable to Meltdown (e.g. AMD)

Sure.

> 
>>  > Default: `true` everywhere else
>>  
>>  Override default selection of whether to isolate 64-bit PV guest page
>>  tables.
>>  
>> +`true` activates page table isolation even on hardware not vulnerable by
>> +Meltdown for all domains.
>> +
>> +`false` deactivates page table isolation on all systems for all domains.
>> +
>> +`default` sets the default behaviour.
>> +
>> +With `dom0` and `domu` it is possible to control page table isolation
>> +for dom0 or guest domains only.
>> +
>>  ### xsave
>>  > `= `
>>  
> 
>> diff --git a/xen/include/asm-x86/spec_ctrl.h 
>> b/xen/include/asm-x86/spec_ctrl.h
>> index 5ab4ff3f68..b4fa43269e 100644
>> --- a/xen/include/asm-x86/spec_ctrl.h
>> +++ b/xen/include/asm-x86/spec_ctrl.h
>> @@ -29,6 +29,10 @@ void init_speculation_mitigations(void);
>>  extern bool opt_ibpb;
>>  extern uint8_t default_bti_ist_info;
>>  
>> +extern uint8_t opt_xpti;
>> +#define OPT_XPTI_DOM0  0x01
>> +#define OPT_XPTI_DOMU  0x02
>> +
>>  static inline void init_shadow_spec_ctrl_state(void)
>>  {
>>  struct cpu_info *info = get_cpu_info();
> 
> Could you please also include something like the following:
> 
> @@ -119,8 +122,9 @@ static void __init print_details(enum ind_thunk thunk)
> boot_cpu_has(X86_FEATURE_RSB_NATIVE)  ? " RSB_NATIVE" : "",
> boot_cpu_has(X86_FEATURE_RSB_VMEXIT)  ? " RSB_VMEXIT" : "");
>  
> -printk("XPTI: %s\n",
> -   boot_cpu_has(X86_FEATURE_NO_XPTI) ? "disabled" : "enabled");
> +printk("XPTI: Dom0 %s, DomU (64-bit PV only) %s\n",
> +   opt_xpti & OPT_XPTI_DOM0 ? "enabled" : "disabled",
> +   opt_xpti & OPT_XPTI_DOMU ? "enabled" : "disabled");
>  }

As stated already in the reply to Jan: I'll use the text:

"XPTI (64-bit PV only): Dom0 ...".

> (just noticed that commit message also needs update regarding param name)

Aah, yes.

Thanks,


Juergen

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

Re: [Xen-devel] [PATCH v8 3/9] xen/x86: support per-domain flag for xpti

[Juergen Gross]

On 18/04/18 11:49, Jan Beulich wrote:
 On 18.04.18 at 11:42,  wrote:
>> On Wed, 2018-04-18 at 10:30 +0200, Juergen Gross wrote:
>> @@ -119,8 +122,9 @@ static void __init print_details(enum ind_thunk thunk)
>> boot_cpu_has(X86_FEATURE_RSB_NATIVE)  ? " RSB_NATIVE" : "",
>> boot_cpu_has(X86_FEATURE_RSB_VMEXIT)  ? " RSB_VMEXIT" : "");
>>  
>> -printk("XPTI: %s\n",
>> -   boot_cpu_has(X86_FEATURE_NO_XPTI) ? "disabled" : "enabled");
>> +printk("XPTI: Dom0 %s, DomU (64-bit PV only) %s\n",
>> +   opt_xpti & OPT_XPTI_DOM0 ? "enabled" : "disabled",
>> +   opt_xpti & OPT_XPTI_DOMU ? "enabled" : "disabled");
>>  }
> 
> Ah, yes, making this log message more precise is certainly worthwhile. The
> placement of "(64-bit PV only)" is a little confusing though: I'd put it 
> either
> before the colon or at the end of the line, as it affects Dom0 as much as
> DomU.

I'd prefer the former.


Juergen


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

Re: [Xen-devel] [PATCH v8 3/9] xen/x86: support per-domain flag for xpti

[Jan Beulich]

>>> On 18.04.18 at 11:42,  wrote:
> On Wed, 2018-04-18 at 10:30 +0200, Juergen Gross wrote:
> @@ -119,8 +122,9 @@ static void __init print_details(enum ind_thunk thunk)
> boot_cpu_has(X86_FEATURE_RSB_NATIVE)  ? " RSB_NATIVE" : "",
> boot_cpu_has(X86_FEATURE_RSB_VMEXIT)  ? " RSB_VMEXIT" : "");
>  
> -printk("XPTI: %s\n",
> -   boot_cpu_has(X86_FEATURE_NO_XPTI) ? "disabled" : "enabled");
> +printk("XPTI: Dom0 %s, DomU (64-bit PV only) %s\n",
> +   opt_xpti & OPT_XPTI_DOM0 ? "enabled" : "disabled",
> +   opt_xpti & OPT_XPTI_DOMU ? "enabled" : "disabled");
>  }

Ah, yes, making this log message more precise is certainly worthwhile. The
placement of "(64-bit PV only)" is a little confusing though: I'd put it either
before the colon or at the end of the line, as it affects Dom0 as much as
DomU.

Jan



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

Re: [Xen-devel] [PATCH v8 3/9] xen/x86: support per-domain flag for xpti

[Sergey Dyasli]

Hi Juergen,

2 small requests from me below.

On Wed, 2018-04-18 at 10:30 +0200, Juergen Gross wrote:
> Instead of switching XPTI globally on or off add a per-domain flag for
> that purpose. This allows to modify the xpti boot parameter to support
> running dom0 without Meltdown mitigations. Using "xpti=nodom0" as boot
> parameter will achieve that.
> 
> Move the xpti boot parameter handling to xen/arch/x86/pv/domain.c as
> it is pv-domain specific.
> 
> Signed-off-by: Juergen Gross 
> Reviewed-by: Jan Beulich 


> diff --git a/docs/misc/xen-command-line.markdown 
> b/docs/misc/xen-command-line.markdown
> index b353352adf..d4f758487a 100644
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1955,14 +1955,24 @@ clustered mode.  The default, given no hint from the 
> **FADT**, is cluster
>  mode.
>  
>  ### xpti
> -> `= `
> +> `= List of [ default |  | dom0= | domu= ]`
>  
> -> Default: `false` on AMD hardware
> +> Default: `false` on hardware not vulnerable to Meltdown (e.g. AMD)

Could this line please be changed to:

`false` on hardware known not to be vulnerable to Meltdown (e.g. AMD)

>  > Default: `true` everywhere else
>  
>  Override default selection of whether to isolate 64-bit PV guest page
>  tables.
>  
> +`true` activates page table isolation even on hardware not vulnerable by
> +Meltdown for all domains.
> +
> +`false` deactivates page table isolation on all systems for all domains.
> +
> +`default` sets the default behaviour.
> +
> +With `dom0` and `domu` it is possible to control page table isolation
> +for dom0 or guest domains only.
> +
>  ### xsave
>  > `= `
>  

> diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
> index 5ab4ff3f68..b4fa43269e 100644
> --- a/xen/include/asm-x86/spec_ctrl.h
> +++ b/xen/include/asm-x86/spec_ctrl.h
> @@ -29,6 +29,10 @@ void init_speculation_mitigations(void);
>  extern bool opt_ibpb;
>  extern uint8_t default_bti_ist_info;
>  
> +extern uint8_t opt_xpti;
> +#define OPT_XPTI_DOM0  0x01
> +#define OPT_XPTI_DOMU  0x02
> +
>  static inline void init_shadow_spec_ctrl_state(void)
>  {
>  struct cpu_info *info = get_cpu_info();

Could you please also include something like the following:

@@ -119,8 +122,9 @@ static void __init print_details(enum ind_thunk thunk)
boot_cpu_has(X86_FEATURE_RSB_NATIVE)  ? " RSB_NATIVE" : "",
boot_cpu_has(X86_FEATURE_RSB_VMEXIT)  ? " RSB_VMEXIT" : "");
 
-printk("XPTI: %s\n",
-   boot_cpu_has(X86_FEATURE_NO_XPTI) ? "disabled" : "enabled");
+printk("XPTI: Dom0 %s, DomU (64-bit PV only) %s\n",
+   opt_xpti & OPT_XPTI_DOM0 ? "enabled" : "disabled",
+   opt_xpti & OPT_XPTI_DOMU ? "enabled" : "disabled");
 }


(just noticed that commit message also needs update regarding param name)

-- 
Thanks,
Sergey
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel