Re: [Xen-devel] [PATCH 3/3] x86/mm: Consolidate all Xen L4 slot writing into init_xen_l4_slots()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 12/10/17 16:08, Jan Beulich wrote:
>>>> On 12.10.17 at 15:54, <andrew.cooper3@xxxxxxxxxx> wrote:
>> --- a/xen/arch/x86/mm/hap/hap.c
>> +++ b/xen/arch/x86/mm/hap/hap.c
>> @@ -391,41 +391,24 @@ int hap_set_allocation(struct domain *d, unsigned int 
>> pages, bool *preempted)
>>      return 0;
>>  }
>>  
>> -static void hap_install_xen_entries_in_l4(struct vcpu *v, mfn_t l4mfn)
>> -{
>> -    struct domain *d = v->domain;
>> -    l4_pgentry_t *l4e;
>> -
>> -    l4e = map_domain_page(l4mfn);
>> -
>> -    /* Copy the common Xen mappings from the idle domain */
>> -    memcpy(&l4e[ROOT_PAGETABLE_FIRST_XEN_SLOT],
>> -           &idle_pg_table[ROOT_PAGETABLE_FIRST_XEN_SLOT],
>> -           ROOT_PAGETABLE_XEN_SLOTS * sizeof(l4_pgentry_t));
>> -
>> -    /* Install the per-domain mappings for this domain */
>> -    l4e[l4_table_offset(PERDOMAIN_VIRT_START)] =
>> -        l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
>> -
>> -    /* Install a linear mapping */
>> -    l4e[l4_table_offset(LINEAR_PT_VIRT_START)] =
>> -        l4e_from_mfn(l4mfn, __PAGE_HYPERVISOR_RW);
>> -
>> -    unmap_domain_page(l4e);
>> -}
>> -
>>  static mfn_t hap_make_monitor_table(struct vcpu *v)
>>  {
>>      struct domain *d = v->domain;
>>      struct page_info *pg;
>> +    l4_pgentry_t *l4e;
>>      mfn_t m4mfn;
>>  
>>      ASSERT(pagetable_get_pfn(v->arch.monitor_table) == 0);
>>  
>>      if ( (pg = hap_alloc(d)) == NULL )
>>          goto oom;
>> +
>>      m4mfn = page_to_mfn(pg);
>> -    hap_install_xen_entries_in_l4(v, m4mfn);
>> +    l4e = __map_domain_page(pg);
> If you obtain the MFN anyway, map_domain_page() is cheaper
> generated code wise.

Ah yes.  Given that __map_domain_page() is a define, I'd hope the
compiler can spot and optimise away the double page_to_mfn().

Either way, fixed.

>
>> --- a/xen/arch/x86/pv/domain.c
>> +++ b/xen/arch/x86/pv/domain.c
>> @@ -35,7 +35,7 @@ static int setup_compat_l4(struct vcpu *v)
>>  
>>      l4tab = __map_domain_page(pg);
>>      clear_page(l4tab);
>> -    init_guest_l4_table(l4tab, v->domain, 1);
>> +    init_xen_l4_slots(l4tab, page_to_mfn(pg), v->domain, INVALID_MFN, 
>> false);
> Perhaps worth avoiding the double translation here too.
>
> In any event
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Just to confirm, the additional delta is:

andrewcoop@andrewcoop:/local/xen.git/xen$ git diff
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 1e7e8d0..41deb90 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -404,7 +404,7 @@ static mfn_t hap_make_monitor_table(struct vcpu *v)
         goto oom;
 
     m4mfn = page_to_mfn(pg);
-    l4e = __map_domain_page(pg);
+    l4e = map_domain_page(m4mfn);
 
     init_xen_l4_slots(l4e, m4mfn, d, INVALID_MFN, false);
     unmap_domain_page(l4e);
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index a242037..2fb1996 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -28,14 +28,16 @@ static int setup_compat_l4(struct vcpu *v)
 {
     struct page_info *pg;
     l4_pgentry_t *l4tab;
+    mfn_t mfn;
 
     pg = alloc_domheap_page(v->domain, MEMF_no_owner);
     if ( pg == NULL )
         return -ENOMEM;
 
-    l4tab = __map_domain_page(pg);
+    mfn = page_to_mfn(pg);
+    l4tab = map_domain_page(mfn);
     clear_page(l4tab);
-    init_xen_l4_slots(l4tab, page_to_mfn(pg), v->domain, INVALID_MFN, false);
+    init_xen_l4_slots(l4tab, mfn, v->domain, INVALID_MFN, false);
     unmap_domain_page(l4tab);
 
     /* This page needs to look like a pagetable so that it can be shadowed */


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel