Re: [PATCH 4/4] x86/PV: properly set shadow allocation for Dom0

2021-08-31 Thread Tim Deegan
At 15:03 +0200 on 30 Aug (1630335824), Jan Beulich wrote:
> Leaving shadow setup just to the L1TF tasklet means running Dom0 on a
> minimally acceptable shadow memory pool, rather than what normally
> would be used (also, for example, for PVH). Populate the pool before
> triggering the tasklet, on a best effort basis (again like done for
> PVH).
> 
> Signed-off-by: Jan Beulich 

Acked-by: Tim Deegan 



Re: [PATCH 4/4] x86/PV: properly set shadow allocation for Dom0

2021-08-31 Thread Jan Beulich
On 31.08.2021 15:47, Andrew Cooper wrote:
> On 30/08/2021 14:03, Jan Beulich wrote:
>> @@ -933,7 +934,17 @@ int __init dom0_construct_pv(struct doma
>>  #ifdef CONFIG_SHADOW_PAGING
>>  if ( opt_dom0_shadow )
>>  {
>> +bool preempted;
>> +
>>  printk("Switching dom0 to using shadow paging\n");
>> +
>> +do {
>> +preempted = false;
>> +shadow_set_allocation(d, dom0_paging_pages(d, nr_pages),
>> +  &preempted);
>> +process_pending_softirqs();
>> +} while ( preempted );
> 
> This isn't correct.  The shadow pool is needed even without
> opt_dom0_shadow, because some downstreams have elected not to retain
> upstream's security vulnerability in default setting of opt_pv_l1tf_hwdom.

Are you suggesting to set up a (perhaps large) shadow pool just in
case we need to enable shadow mode on Dom0? And all of this memory
to then remain unused in the majority of cases?

Plus even if so, I'd view this as a 2nd, independent step, largely
orthogonal to the handling of "dom0=shadow". If somebody really
wanted that, I think this should be driven by an explicit setting
of the shadow pool size, indicating the admin is willing to waste
the memory.

I'm further puzzled by "not to retain upstream's security
vulnerability" - are you saying upstream is vulnerable in some way,
while perhaps you (XenServer) are not? In general I don't think I
view downstream decisions as a driving factor for what upstream
does, when the result is deliberately different behavior from
upstream.

> Also, dom0_paging_pages() isn't a trivial calculation, so should be
> called once and cached.

Sure, can do that. You did notice though that all I did is take
PVH's similar code?

Jan




Re: [PATCH 4/4] x86/PV: properly set shadow allocation for Dom0

2021-08-31 Thread Andrew Cooper
On 30/08/2021 14:03, Jan Beulich wrote:
> @@ -933,7 +934,17 @@ int __init dom0_construct_pv(struct doma
>  #ifdef CONFIG_SHADOW_PAGING
>  if ( opt_dom0_shadow )
>  {
> +bool preempted;
> +
>  printk("Switching dom0 to using shadow paging\n");
> +
> +do {
> +preempted = false;
> +shadow_set_allocation(d, dom0_paging_pages(d, nr_pages),
> +  &preempted);
> +process_pending_softirqs();
> +} while ( preempted );

This isn't correct.  The shadow pool is needed even without
opt_dom0_shadow, because some downstreams have elected not to retain
upstream's security vulnerability in default setting of opt_pv_l1tf_hwdom.

Also, dom0_paging_pages() isn't a trivial calculation, so should be
called once and cached.

~Andrew