diff mbox series

[10/22] x86/mapcache: initialise the mapcache for the idle domain

Message ID 20221216114853.8227-11-julien@xen.org (mailing list archive)
State New
Headers show
Series Remove the directmap | expand

Commit Message

Julien Grall Dec. 16, 2022, 11:48 a.m. UTC 
From: Hongyan Xia <hongyxia@amazon.com>

In order to use the mapcache in the idle domain, we also have to
populate its page tables in the PERDOMAIN region, and we need to move
mapcache_domain_init() earlier in arch_domain_create().

Note, commit 'x86: lift mapcache variable to the arch level' has
initialised the mapcache for HVM domains. With this patch, PV, HVM,
idle domains now all initialise the mapcache.

Signed-off-by: Wei Wang <wawei@amazon.de>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/x86/domain.c | 4 ++--
 xen/arch/x86/mm.c     | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Jan Beulich Dec. 22, 2022, 1:06 p.m. UTC | #1 
On 16.12.2022 12:48, Julien Grall wrote:
> From: Hongyan Xia <hongyxia@amazon.com>
> 
> In order to use the mapcache in the idle domain, we also have to
> populate its page tables in the PERDOMAIN region, and we need to move
> mapcache_domain_init() earlier in arch_domain_create().
> 
> Note, commit 'x86: lift mapcache variable to the arch level' has
> initialised the mapcache for HVM domains. With this patch, PV, HVM,
> idle domains now all initialise the mapcache.

But they can't use it yet, can they? This needs saying explicitly, or
else one is going to make wrong implications.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -732,6 +732,8 @@ int arch_domain_create(struct domain *d,
>  
>      spin_lock_init(&d->arch.e820_lock);
>  
> +    mapcache_domain_init(d);
> +
>      /* Minimal initialisation for the idle domain. */
>      if ( unlikely(is_idle_domain(d)) )
>      {
> @@ -829,8 +831,6 @@ int arch_domain_create(struct domain *d,
>  
>      psr_domain_init(d);
>  
> -    mapcache_domain_init(d);

You move this ahead of error paths taking the "goto out" route, so
adjustments to affected error paths are going to be needed to avoid
memory leaks.

> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -5963,6 +5963,9 @@ int create_perdomain_mapping(struct domain *d, unsigned long va,
>          l3tab = __map_domain_page(pg);
>          clear_page(l3tab);
>          d->arch.perdomain_l3_pg = pg;
> +        if ( is_idle_domain(d) )
> +            idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
> +                l4e_from_page(pg, __PAGE_HYPERVISOR_RW);

Hmm, having an idle domain check here isn't very nice. I agree putting
it in arch_domain_create()'s respective conditional isn't very neat
either, but personally I'd consider this at least a little less bad.
And the layering violation aspect isn't much worse than that of setting
d->arch.ctxt_switch there as well.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 069b7d2af330..ec150f4fd144 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -732,6 +732,8 @@  int arch_domain_create(struct domain *d,
 
     spin_lock_init(&d->arch.e820_lock);
 
+    mapcache_domain_init(d);
+
     /* Minimal initialisation for the idle domain. */
     if ( unlikely(is_idle_domain(d)) )
     {
@@ -829,8 +831,6 @@  int arch_domain_create(struct domain *d,
 
     psr_domain_init(d);
 
-    mapcache_domain_init(d);
-
     if ( is_hvm_domain(d) )
     {
         if ( (rc = hvm_domain_initialise(d, config)) != 0 )
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 8b9740f57519..041bd4cfde17 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -5963,6 +5963,9 @@  int create_perdomain_mapping(struct domain *d, unsigned long va,
         l3tab = __map_domain_page(pg);
         clear_page(l3tab);
         d->arch.perdomain_l3_pg = pg;
+        if ( is_idle_domain(d) )
+            idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
+                l4e_from_page(pg, __PAGE_HYPERVISOR_RW);
         if ( !nr )
         {
             unmap_domain_page(l3tab);