[PATCH] x86: idle domains don't have a domain-page mapcache

[PATCH] x86: idle domains don't have a domain-page mapcache

[Jan Beulich]

First and foremost correct a comment implying the opposite. Then, to
make things more clear PV-vs-HVM-wise, move the PV check earlier in the
function, making it unnecessary for both callers to perform the check
individually. Finally return NULL from the function when using the idle
domain's page tables, allowing a dcache->inuse check to also become an
assertion.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -28,8 +28,11 @@ static inline struct vcpu *mapcache_curr
     /*
      * When current isn't properly set up yet, this is equivalent to
      * running in an idle vCPU (callers must check for NULL).
+     *
+     * Non-PV domains don't have any mapcache.  For idle domains (which
+     * appear to be PV but also have no mapcache) see below.
      */
-    if ( !v )
+    if ( !v || !is_pv_vcpu(v) )
         return NULL;
 
     /*
@@ -41,19 +44,22 @@ static inline struct vcpu *mapcache_curr
         return NULL;
 
     /*
-     * If guest_table is NULL, and we are running a paravirtualised guest,
-     * then it means we are running on the idle domain's page table and must
-     * therefore use its mapcache.
+     * If guest_table is NULL for a PV domain (which includes IDLE), then it
+     * means we are running on the idle domain's page tables and therefore
+     * must not use any mapcache.
      */
-    if ( unlikely(pagetable_is_null(v->arch.guest_table)) && is_pv_vcpu(v) )
+    if ( unlikely(pagetable_is_null(v->arch.guest_table)) )
     {
         /* If we really are idling, perform lazy context switch now. */
-        if ( (v = idle_vcpu[smp_processor_id()]) == current )
+        if ( idle_vcpu[smp_processor_id()] == current )
             sync_local_execstate();
         /* We must now be running on the idle page table. */
         ASSERT(cr3_pa(read_cr3()) == __pa(idle_pg_table));
+        return NULL;
     }
 
+    ASSERT(!is_idle_vcpu(v));
+
     return v;
 }
 
@@ -82,13 +88,12 @@ void *map_domain_page(mfn_t mfn)
 #endif
 
     v = mapcache_current_vcpu();
-    if ( !v || !is_pv_vcpu(v) )
+    if ( !v )
         return mfn_to_virt(mfn_x(mfn));
 
     dcache = &v->domain->arch.pv.mapcache;
     vcache = &v->arch.pv.mapcache;
-    if ( !dcache->inuse )
-        return mfn_to_virt(mfn_x(mfn));
+    ASSERT(dcache->inuse);
 
     perfc_incr(map_domain_page_count);
 
@@ -187,7 +192,7 @@ void unmap_domain_page(const void *ptr)
     ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END);
 
     v = mapcache_current_vcpu();
-    ASSERT(v && is_pv_vcpu(v));
+    ASSERT(v);
 
     dcache = &v->domain->arch.pv.mapcache;
     ASSERT(dcache->inuse);

Re: [PATCH] x86: idle domains don't have a domain-page mapcache

[Andrew Cooper]

On 05/01/2023 11:09 am, Jan Beulich wrote:
> First and foremost correct a comment implying the opposite. Then, to
> make things more clear PV-vs-HVM-wise, move the PV check earlier in the
> function, making it unnecessary for both callers to perform the check
> individually. Finally return NULL from the function when using the idle
> domain's page tables, allowing a dcache->inuse check to also become an
> assertion.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

By and large, I think these changes ok, but I want to make an argument
for going further right away.


Most of mapcache_current_vcpu() is a giant bodge to support the weird
context in dom0_construct_pv(), but we pay the price unilaterally.

By updating current too in that weird context, I think we can drop
mapcache_override_current().  It's also worth noting that the only
callers are __init so having this_cpu(override) as per-cpu is a waste.

But I also think we can drop the pagetable_is_null(v->arch.guest_table)
part too.  If we happen to be half-idle, it doesn't matter if we use the
current mapcache by following cpu_info()->current instead.  That said, I
can't think of any case where we could legally access transient mappings
from an idle context, so I'd be tempted to see if we can simply drop
that clause.


Overall, I think the logic would benefit from being expressed in terms
of short_directmap, just like init_xen_l4_slots().  That is ultimately
the property that we care about, and it's also the property that is
changing in the effort to remove the directmap entirely.

If not short_directmap, then at least having the property expressed
consistently between the two piece of code, seeing as it is the same
property.

~Andrew

Re: [PATCH] x86: idle domains don't have a domain-page mapcache

[Julien Grall]

Hi Jan,

On 05/01/2023 11:09, Jan Beulich wrote:
> First and foremost correct a comment implying the opposite. Then, to
> make things more clear PV-vs-HVM-wise, move the PV check earlier in the
> function, making it unnecessary for both callers to perform the check
> individually. Finally return NULL from the function when using the idle
> domain's page tables, allowing a dcache->inuse check to also become an
> assertion.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/domain_page.c
> +++ b/xen/arch/x86/domain_page.c
> @@ -28,8 +28,11 @@ static inline struct vcpu *mapcache_curr
>       /*
>        * When current isn't properly set up yet, this is equivalent to
>        * running in an idle vCPU (callers must check for NULL).
> +     *
> +     * Non-PV domains don't have any mapcache.  For idle domains (which
> +     * appear to be PV but also have no mapcache) see below.
>        */
> -    if ( !v )
> +    if ( !v || !is_pv_vcpu(v) )
>           return NULL;

I am trying to figure out the implication on this change with my patch 
[1]. Is this expected to go before hand?

If so, is this expectation that I will remove !is_pv_vcpu() and replace 
with a new check...

>   
>       /*
> @@ -41,19 +44,22 @@ static inline struct vcpu *mapcache_curr
>           return NULL;

... right here?

if ( !is_pv_vcpu(v) )
   return v;


>   
>       /*
> -     * If guest_table is NULL, and we are running a paravirtualised guest,
> -     * then it means we are running on the idle domain's page table and must
> -     * therefore use its mapcache.
> +     * If guest_table is NULL for a PV domain (which includes IDLE), then it
> +     * means we are running on the idle domain's page tables and therefore
> +     * must not use any mapcache.
>        */
> -    if ( unlikely(pagetable_is_null(v->arch.guest_table)) && is_pv_vcpu(v) )
> +    if ( unlikely(pagetable_is_null(v->arch.guest_table)) )
>       {
>           /* If we really are idling, perform lazy context switch now. */
> -        if ( (v = idle_vcpu[smp_processor_id()]) == current )
> +        if ( idle_vcpu[smp_processor_id()] == current )
>               sync_local_execstate();
>           /* We must now be running on the idle page table. */
>           ASSERT(cr3_pa(read_cr3()) == __pa(idle_pg_table));
> +        return NULL;
>       }
>   
> +    ASSERT(!is_idle_vcpu(v));
> +
>       return v;
>   }
>   
> @@ -82,13 +88,12 @@ void *map_domain_page(mfn_t mfn)
>   #endif
>   
>       v = mapcache_current_vcpu();
> -    if ( !v || !is_pv_vcpu(v) )
> +    if ( !v )
>           return mfn_to_virt(mfn_x(mfn));
>   
>       dcache = &v->domain->arch.pv.mapcache;
>       vcache = &v->arch.pv.mapcache;
> -    if ( !dcache->inuse )
> -        return mfn_to_virt(mfn_x(mfn));
> +    ASSERT(dcache->inuse);
>   
>       perfc_incr(map_domain_page_count);
>   
> @@ -187,7 +192,7 @@ void unmap_domain_page(const void *ptr)
>       ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END);
>   
>       v = mapcache_current_vcpu();
> -    ASSERT(v && is_pv_vcpu(v));
> +    ASSERT(v);
>   
>       dcache = &v->domain->arch.pv.mapcache;
>       ASSERT(dcache->inuse);

Cheers,

[1] https://lore.kernel.org/xen-devel/20221216114853.8227-15-julien@xen.org

-- 
Julien Grall

Re: [PATCH] x86: idle domains don't have a domain-page mapcache

[Jan Beulich]

On 30.01.2023 20:00, Julien Grall wrote:
> On 05/01/2023 11:09, Jan Beulich wrote:
>> --- a/xen/arch/x86/domain_page.c
>> +++ b/xen/arch/x86/domain_page.c
>> @@ -28,8 +28,11 @@ static inline struct vcpu *mapcache_curr
>>       /*
>>        * When current isn't properly set up yet, this is equivalent to
>>        * running in an idle vCPU (callers must check for NULL).
>> +     *
>> +     * Non-PV domains don't have any mapcache.  For idle domains (which
>> +     * appear to be PV but also have no mapcache) see below.
>>        */
>> -    if ( !v )
>> +    if ( !v || !is_pv_vcpu(v) )
>>           return NULL;
> 
> I am trying to figure out the implication on this change with my patch 
> [1]. Is this expected to go before hand?

The change here may not be necessary at all with your change. The main
reason I sent this patch was to augment my comments on your patch. If
yours gets adjusted to account for the (perhaps just theoretical) issues
(if just theoretical, then description adjustments may be all that's
needed), then I may be able to drop this instead of seeing whether I can
convince myself that some / all of Andrew's requests can actually safely
be fulfilled.

Jan