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

[Juergen Gross <jgross@xxxxxxxx>]

On 10/04/18 11:14, Jan Beulich wrote:
>>>> On 10.04.18 at 09:58, <jgross@xxxxxxxx> wrote:
>> --- a/docs/misc/xen-command-line.markdown
>> +++ b/docs/misc/xen-command-line.markdown
>> @@ -1955,14 +1955,29 @@ clustered mode.  The default, given no hint from the 
>> **FADT**, is cluster
>>  mode.
>>  
>>  ### xpti
>> -> `= <boolean>`
>> +> `= List of [ default | <boolean> | dom0=<bool> | domu=<bool> ]`
>>  
>> -> Default: `false` on AMD hardware
>> +> Default: `false` on hardware not 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.
>> +
>> +`dom0=false` deactivates page table isolation for dom0.
>> +
>> +`dom0=true` activates page table isolation for dom0.
>> +
>> +`domu=false` deactivates page table isolation for guest domains.
>> +
>> +`domu=true` activates page table isolation for guest domains.
> 
> This is too verbose / repetitive for my taste.

So you'd like it better as:

"With `dom0` and `domu` it is possible to control page table isolation
for dom0 or guest domains only." ?

> 
>> @@ -205,6 +208,10 @@ int pv_domain_initialise(struct domain *d)
>>      /* 64-bit PV guest by default. */
>>      d->arch.is_32bit_pv = d->arch.has_32bit_shinfo = 0;
>>  
>> +    d->arch.pv_domain.xpti = (d->domain_id == hardware_domid)
>> +                             ? (opt_xpti & XPTI_DOM0)
>> +                             : (opt_xpti & XPTI_DOMU);
> 
> I would generally prefer to have as little redundancy as possible in
> such expressions, i.e.
> 
>     d->arch.pv_domain.xpti = opt_xpti & (d->domain_id == hardware_domid
>                                          ? XPTI_DOM0 : XPTI_DOMU);

Okay.

> 
> Furthermore - shouldn't this cover domain 0 as well as the hardware
> domain, even if - in case they are different - domain 0 should be
> short lived?

When domain 0 is created is _is_ the hardware domain. Only domain 0
creating a hardware domain will set hardware_domid to a non-zero value.

> 
>> --- a/xen/arch/x86/spec_ctrl.c
>> +++ b/xen/arch/x86/spec_ctrl.c
>> @@ -193,6 +193,68 @@ static bool __init retpoline_safe(void)
>>     }
>> }
>>
>> +#define XPTI_DEFAULT  0xff
>> +uint8_t opt_xpti = XPTI_DEFAULT;
> 
> __read_mostly

Okay.

> 
>> --- 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 XPTI_DOM0  0x01
>> +#define XPTI_DOMU  0x02
> 
> OPT_XPTI_DOM{0,U} would perhaps have been better.
> 
> Anyway, in the interest of getting done with this
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with or without some or all of the suggestions addressed.

I can send a followup patch.


Juergen

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