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, archived
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
Elias El Yandouzi Jan. 10, 2024, 4:24 p.m. UTC | #2 
Hi,

On 22/12/2022 13:06, Jan Beulich wrote:
> 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.
> 

Yes, I tried to use the mapcache right after the idle vCPU gets 
scheduled and it worked. So, I believe it is enough.

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

Correct, I'll fix that.

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

Why do you think it would be less bad to move it in 
arch_domain_create()? To me, it would make things worse as it would 
spread the mapping stuff across different functions.
Jan Beulich Jan. 11, 2024, 7:53 a.m. UTC | #3 
On 10.01.2024 17:24, Elias El Yandouzi wrote:
> On 22/12/2022 13:06, Jan Beulich wrote:
>> On 16.12.2022 12:48, Julien Grall wrote:
>>> --- 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.
> 
> Why do you think it would be less bad to move it in 
> arch_domain_create()? To me, it would make things worse as it would 
> spread the mapping stuff across different functions.

Not sure what to add to what I said: create_perdomain_mapping() gaining
such a check is a layering violation to me. arch_domain_create() otoh
special cases the idle domain already.

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);