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 <jbeulich@suse.com>
Acked-by: Tim Deegan <tim@xen.org>
---
v2: Latch dom0_paging_pages() result.

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1298,7 +1298,7 @@ int shadow_set_allocation(struct domain
 {
     struct page_info *sp;
 
-    ASSERT(paging_locked_by_me(d));
+    ASSERT(paging_locked_by_me(d) || system_state < SYS_STATE_active);
 
     if ( pages > 0 )
     {
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -21,6 +21,7 @@
 #include <asm/page.h>
 #include <asm/pv/mm.h>
 #include <asm/setup.h>
+#include <asm/shadow.h>
 
 /* Allow ring-3 access in long mode as guest cannot use ring 1 ... */
 #define BASE_PROT (_PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_USER)
@@ -933,7 +934,18 @@ 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");
+
+        nr_pt_pages = dom0_paging_pages(d, nr_pages);
+
+        do {
+            preempted = false;
+            shadow_set_allocation(d, nr_pt_pages, &preempted);
+            process_pending_softirqs();
+        } while ( preempted );
+
         tasklet_schedule(&d->arch.paging.shadow.pv_l1tf_tasklet);
     }
 #endif

On 21/09/2021 08:17, 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 <jbeulich@suse.com>
> Acked-by: Tim Deegan <tim@xen.org>
> ---
> v2: Latch dom0_paging_pages() result.
>
> --- a/xen/arch/x86/mm/shadow/common.c
> +++ b/xen/arch/x86/mm/shadow/common.c
> @@ -1298,7 +1298,7 @@ int shadow_set_allocation(struct domain
>  {
>      struct page_info *sp;
>  
> -    ASSERT(paging_locked_by_me(d));
> +    ASSERT(paging_locked_by_me(d) || system_state < SYS_STATE_active);
>  
>      if ( pages > 0 )
>      {
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -21,6 +21,7 @@
>  #include <asm/page.h>
>  #include <asm/pv/mm.h>
>  #include <asm/setup.h>
> +#include <asm/shadow.h>
>  
>  /* Allow ring-3 access in long mode as guest cannot use ring 1 ... */
>  #define BASE_PROT (_PAGE_PRESENT|_PAGE_RW|_PAGE_ACCESSED|_PAGE_USER)
> @@ -933,7 +934,18 @@ 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");
> +
> +        nr_pt_pages = dom0_paging_pages(d, nr_pages);
> +
> +        do {
> +            preempted = false;
> +            shadow_set_allocation(d, nr_pt_pages, &preempted);
> +            process_pending_softirqs();
> +        } while ( preempted );

This is still broken.

The loop setting the shadow allocation needs to be outside of this
conditional, because it is not related to early activation of the l1tf
tasklet.

~Andrew

On 22.09.2021 15:31, Andrew Cooper wrote:
> On 21/09/2021 08:17, Jan Beulich wrote:
>> @@ -933,7 +934,18 @@ 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");
>> +
>> +        nr_pt_pages = dom0_paging_pages(d, nr_pages);
>> +
>> +        do {
>> +            preempted = false;
>> +            shadow_set_allocation(d, nr_pt_pages, &preempted);
>> +            process_pending_softirqs();
>> +        } while ( preempted );
> 
> This is still broken.
> 
> The loop setting the shadow allocation needs to be outside of this
> conditional, because it is not related to early activation of the l1tf
> tasklet.

Well, I'm not sure what to say. On v1 you already said so. But then you
didn't care to reply to me responding:

"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."

Which has left me with no justification to make the change you're
requesting. I've now got an ack by Tim and an R-b by Roger. I also
view the change as is being an improvement on its own (i.e. I
question you saying "This is still broken."), even if (later) we
were to follow what you request. For this reason I'll give it a day
or two for you to reply, but otherwise I'll commit the patch as is,
leaving further adjustments for a future change (by you, me, or
anyone else).

Jan

On Wed, Sep 22, 2021 at 03:50:25PM +0200, Jan Beulich wrote:
> On 22.09.2021 15:31, Andrew Cooper wrote:
> > On 21/09/2021 08:17, Jan Beulich wrote:
> >> @@ -933,7 +934,18 @@ 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");
> >> +
> >> +        nr_pt_pages = dom0_paging_pages(d, nr_pages);
> >> +
> >> +        do {
> >> +            preempted = false;
> >> +            shadow_set_allocation(d, nr_pt_pages, &preempted);
> >> +            process_pending_softirqs();
> >> +        } while ( preempted );
> > 
> > This is still broken.
> > 
> > The loop setting the shadow allocation needs to be outside of this
> > conditional, because it is not related to early activation of the l1tf
> > tasklet.
> 
> Well, I'm not sure what to say. On v1 you already said so. But then you
> didn't care to reply to me responding:
> 
> "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.

Maybe an acceptable compromise would be to allocate the pool if
opt_dom0_shadow || opt_pv_l1tf_hwdom?

opt_pv_l1tf_hwdom is not enabled by default, so an admin opting to
enable it should also be willing to reserve the memory it requires in
case it needs activating during runtime.

Roger.

On 22.09.2021 16:25, Roger Pau Monné wrote:
> On Wed, Sep 22, 2021 at 03:50:25PM +0200, Jan Beulich wrote:
>> On 22.09.2021 15:31, Andrew Cooper wrote:
>>> On 21/09/2021 08:17, Jan Beulich wrote:
>>>> @@ -933,7 +934,18 @@ 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");
>>>> +
>>>> +        nr_pt_pages = dom0_paging_pages(d, nr_pages);
>>>> +
>>>> +        do {
>>>> +            preempted = false;
>>>> +            shadow_set_allocation(d, nr_pt_pages, &preempted);
>>>> +            process_pending_softirqs();
>>>> +        } while ( preempted );
>>>
>>> This is still broken.
>>>
>>> The loop setting the shadow allocation needs to be outside of this
>>> conditional, because it is not related to early activation of the l1tf
>>> tasklet.
>>
>> Well, I'm not sure what to say. On v1 you already said so. But then you
>> didn't care to reply to me responding:
>>
>> "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.
> 
> Maybe an acceptable compromise would be to allocate the pool if
> opt_dom0_shadow || opt_pv_l1tf_hwdom?
> 
> opt_pv_l1tf_hwdom is not enabled by default, so an admin opting to
> enable it should also be willing to reserve the memory it requires in
> case it needs activating during runtime.

I'd be fine making that change. Andrew - would that be sufficient to
address your concern?

Jan

On Tue, Sep 21, 2021 at 09:17:15AM +0200, 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 <jbeulich@suse.com>
> Acked-by: Tim Deegan <tim@xen.org>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.