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>
Signed-off-by: Elias El Yandouzi <eliasely@amazon.com>

----

	Changes in V2:
          * Free resources if mapcache initialisation fails
          * Remove `is_idle_domain()` check from `create_perdomain_mappings()`

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8ef3f7746f..d4c125bc14 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -750,9 +750,16 @@ int arch_domain_create(struct domain *d,
 
     spin_lock_init(&d->arch.e820_lock);
 
+    if ( (rc = mapcache_domain_init(d)) != 0)
+    {
+        free_perdomain_mappings(d);
+        return rc;
+    }
+
     /* Minimal initialisation for the idle domain. */
     if ( unlikely(is_idle_domain(d)) )
     {
+        struct page_info *pg = d->arch.perdomain_l3_pg;
         static const struct arch_csw idle_csw = {
             .from = paravirt_ctxt_switch_from,
             .to   = paravirt_ctxt_switch_to,
@@ -763,6 +770,9 @@ int arch_domain_create(struct domain *d,
 
         d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */
 
+        idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
+            l4e_from_page(pg, __PAGE_HYPERVISOR_RW);
+
         return 0;
     }
 
@@ -843,8 +853,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 )
-- 
2.40.1

On 16.01.2024 20:25, Elias El Yandouzi wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -750,9 +750,16 @@ int arch_domain_create(struct domain *d,
>  
>      spin_lock_init(&d->arch.e820_lock);
>  
> +    if ( (rc = mapcache_domain_init(d)) != 0)
> +    {
> +        free_perdomain_mappings(d);
> +        return rc;
> +    }
> +
>      /* Minimal initialisation for the idle domain. */
>      if ( unlikely(is_idle_domain(d)) )
>      {
> +        struct page_info *pg = d->arch.perdomain_l3_pg;
>          static const struct arch_csw idle_csw = {
>              .from = paravirt_ctxt_switch_from,
>              .to   = paravirt_ctxt_switch_to,
> @@ -763,6 +770,9 @@ int arch_domain_create(struct domain *d,
>  
>          d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */
>  
> +        idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
> +            l4e_from_page(pg, __PAGE_HYPERVISOR_RW);
> +
>          return 0;
>      }

Why not add another call to mapcache_domain_init() right here, allowing
a more specific panic() to be invoked in case of failure (compared to
the BUG_ON() upon failure of creation of the idle domain as a whole)?
Then the other mapcache_domain_init() call doesn't need moving a 2nd
time in close succession.

Jan

On 20/02/2024 10:51, Jan Beulich wrote:
> On 16.01.2024 20:25, Elias El Yandouzi wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -750,9 +750,16 @@ int arch_domain_create(struct domain *d,
>>   
>>       spin_lock_init(&d->arch.e820_lock);
>>   
>> +    if ( (rc = mapcache_domain_init(d)) != 0)
>> +    {
>> +        free_perdomain_mappings(d);
>> +        return rc;
>> +    }
>> +
>>       /* Minimal initialisation for the idle domain. */
>>       if ( unlikely(is_idle_domain(d)) )
>>       {
>> +        struct page_info *pg = d->arch.perdomain_l3_pg;
>>           static const struct arch_csw idle_csw = {
>>               .from = paravirt_ctxt_switch_from,
>>               .to   = paravirt_ctxt_switch_to,
>> @@ -763,6 +770,9 @@ int arch_domain_create(struct domain *d,
>>   
>>           d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */
>>   
>> +        idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
>> +            l4e_from_page(pg, __PAGE_HYPERVISOR_RW);
>> +
>>           return 0;
>>       }
> 
> Why not add another call to mapcache_domain_init() right here, allowing
> a more specific panic() to be invoked in case of failure (compared to
> the BUG_ON() upon failure of creation of the idle domain as a whole)?
> Then the other mapcache_domain_init() call doesn't need moving a 2nd
> time in close succession.
> 

To be honest, I don't really like the idea of having twice the same call 
just for the benefit of having a panic() call in case of failure for the 
idle domain.

If you don't mind, I'd rather keep just a single call to 
mapcache_domain_init() as it is now.

Elias

On 13.05.2024 11:35, Elias El Yandouzi wrote:
> On 20/02/2024 10:51, Jan Beulich wrote:
>> On 16.01.2024 20:25, Elias El Yandouzi wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -750,9 +750,16 @@ int arch_domain_create(struct domain *d,
>>>   
>>>       spin_lock_init(&d->arch.e820_lock);
>>>   
>>> +    if ( (rc = mapcache_domain_init(d)) != 0)
>>> +    {
>>> +        free_perdomain_mappings(d);
>>> +        return rc;
>>> +    }
>>> +
>>>       /* Minimal initialisation for the idle domain. */
>>>       if ( unlikely(is_idle_domain(d)) )
>>>       {
>>> +        struct page_info *pg = d->arch.perdomain_l3_pg;
>>>           static const struct arch_csw idle_csw = {
>>>               .from = paravirt_ctxt_switch_from,
>>>               .to   = paravirt_ctxt_switch_to,
>>> @@ -763,6 +770,9 @@ int arch_domain_create(struct domain *d,
>>>   
>>>           d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */
>>>   
>>> +        idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
>>> +            l4e_from_page(pg, __PAGE_HYPERVISOR_RW);
>>> +
>>>           return 0;
>>>       }
>>
>> Why not add another call to mapcache_domain_init() right here, allowing
>> a more specific panic() to be invoked in case of failure (compared to
>> the BUG_ON() upon failure of creation of the idle domain as a whole)?
>> Then the other mapcache_domain_init() call doesn't need moving a 2nd
>> time in close succession.
> 
> To be honest, I don't really like the idea of having twice the same call 
> just for the benefit of having a panic() call in case of failure for the 
> idle domain.

Resulting in the problem Roger has now validly pointed out in reply to v3.
IOW the (more specific) panic() isn't the only reason; it would merely be
an imo desirable side effect.

Jan

On 20/02/2024 10:51, Jan Beulich wrote:
> On 16.01.2024 20:25, Elias El Yandouzi wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -750,9 +750,16 @@ int arch_domain_create(struct domain *d,
>>   
>>       spin_lock_init(&d->arch.e820_lock);
>>   
>> +    if ( (rc = mapcache_domain_init(d)) != 0)
>> +    {
>> +        free_perdomain_mappings(d);
>> +        return rc;
>> +    }
>> +
>>       /* Minimal initialisation for the idle domain. */
>>       if ( unlikely(is_idle_domain(d)) )
>>       {
>> +        struct page_info *pg = d->arch.perdomain_l3_pg;
>>           static const struct arch_csw idle_csw = {
>>               .from = paravirt_ctxt_switch_from,
>>               .to   = paravirt_ctxt_switch_to,
>> @@ -763,6 +770,9 @@ int arch_domain_create(struct domain *d,
>>   
>>           d->arch.cpu_policy = ZERO_BLOCK_PTR; /* Catch stray misuses. */
>>   
>> +        idle_pg_table[l4_table_offset(PERDOMAIN_VIRT_START)] =
>> +            l4e_from_page(pg, __PAGE_HYPERVISOR_RW);
>> +
>>           return 0;
>>       }
> 
> Why not add another call to mapcache_domain_init() right here, allowing
> a more specific panic() to be invoked in case of failure (compared to
> the BUG_ON() upon failure of creation of the idle domain as a whole)?
> Then the other mapcache_domain_init() call doesn't need moving a 2nd
> time in close succession.


Sorry but I don't get your point, why calling another time 
`mapcache_domain_init()`? What panic() are you referring to?

Elias