diff mbox series

[v7,09/15] efi: use new page table APIs in copy_mapping

Message ID 0259b645c81ecc3879240e30760b0e7641a2b602.1590750232.git.hongyxia@amazon.com (mailing list archive)
State Superseded
Headers show
Series switch to domheap for Xen page tables | expand

Commit Message

Hongyan Xia May 29, 2020, 11:11 a.m. UTC 
From: Wei Liu <wei.liu2@citrix.com>

After inspection ARM doesn't have alloc_xen_pagetable so this function
is x86 only, which means it is safe for us to change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>

---
Changed in v7:
- hoist l3 variables out of the loop to avoid repetitive mappings.
---
 xen/common/efi/boot.c | 34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

Comments

Jan Beulich July 14, 2020, 12:42 p.m. UTC | #1 
On 29.05.2020 13:11, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
> 
> After inspection ARM doesn't have alloc_xen_pagetable so this function
> is x86 only, which means it is safe for us to change.

Well, it sits inside a "#ifndef CONFIG_ARM" section.

> @@ -1442,29 +1443,42 @@ static __init void copy_mapping(unsigned long mfn, unsigned long end,
>                                                   unsigned long emfn))
>  {
>      unsigned long next;
> +    l3_pgentry_t *l3src = NULL, *l3dst = NULL;
>  
>      for ( ; mfn < end; mfn = next )
>      {
>          l4_pgentry_t l4e = efi_l4_pgtable[l4_table_offset(mfn << PAGE_SHIFT)];
> -        l3_pgentry_t *l3src, *l3dst;
>          unsigned long va = (unsigned long)mfn_to_virt(mfn);
>  
> +        if ( !((mfn << PAGE_SHIFT) & ((1UL << L4_PAGETABLE_SHIFT) - 1)) )

To be in line with ...

> +        {
> +            UNMAP_DOMAIN_PAGE(l3src);
> +            UNMAP_DOMAIN_PAGE(l3dst);
> +        }
>          next = mfn + (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT));

... this, please avoid the left shift of mfn in the if(). Judging from
code further down I also think the zapping of l3src would better be
dependent upon va than upon mfn.

>          if ( !is_valid(mfn, min(next, end)) )
>              continue;
> -        if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
> +        if ( !l3dst )
>          {
> -            l3dst = alloc_xen_pagetable();
> -            BUG_ON(!l3dst);
> -            clear_page(l3dst);
> -            efi_l4_pgtable[l4_table_offset(mfn << PAGE_SHIFT)] =
> -                l4e_from_paddr(virt_to_maddr(l3dst), __PAGE_HYPERVISOR);
> +            if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
> +            {
> +                mfn_t l3mfn;
> +
> +                l3dst = alloc_map_clear_xen_pt(&l3mfn);
> +                BUG_ON(!l3dst);
> +                efi_l4_pgtable[l4_table_offset(mfn << PAGE_SHIFT)] =
> +                    l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR);
> +            }
> +            else
> +                l3dst = map_l3t_from_l4e(l4e);
>          }
> -        else
> -            l3dst = l4e_to_l3e(l4e);

As for the earlier patch, maybe again neater if you started with

        if ( l3dst )
            /* nothing */;
        else if ...

Would also save a level of indentation as it seems.

Jan
Hongyan Xia July 27, 2020, 12:45 p.m. UTC | #2 
On Tue, 2020-07-14 at 14:42 +0200, Jan Beulich wrote:
> On 29.05.2020 13:11, Hongyan Xia wrote:
> > From: Wei Liu <wei.liu2@citrix.com>
> > 
> > After inspection ARM doesn't have alloc_xen_pagetable so this
> > function
> > is x86 only, which means it is safe for us to change.
> 
> Well, it sits inside a "#ifndef CONFIG_ARM" section.
> 
> > @@ -1442,29 +1443,42 @@ static __init void copy_mapping(unsigned
> > long mfn, unsigned long end,
> >                                                   unsigned long
> > emfn))
> >  {
> >      unsigned long next;
> > +    l3_pgentry_t *l3src = NULL, *l3dst = NULL;
> >  
> >      for ( ; mfn < end; mfn = next )
> >      {
> >          l4_pgentry_t l4e = efi_l4_pgtable[l4_table_offset(mfn <<
> > PAGE_SHIFT)];
> > -        l3_pgentry_t *l3src, *l3dst;
> >          unsigned long va = (unsigned long)mfn_to_virt(mfn);
> >  
> > +        if ( !((mfn << PAGE_SHIFT) & ((1UL << L4_PAGETABLE_SHIFT)
> > - 1)) )
> 
> To be in line with ...
> 
> > +        {
> > +            UNMAP_DOMAIN_PAGE(l3src);
> > +            UNMAP_DOMAIN_PAGE(l3dst);
> > +        }
> >          next = mfn + (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT));
> 
> ... this, please avoid the left shift of mfn in the if(). Judgingfrom

What do you mean by "in line" here? It does not look to me that "next
=" can be easily squashed into the if() condition.

Hongyan
Hongyan Xia July 27, 2020, 1:45 p.m. UTC | #3 
On Mon, 2020-07-27 at 13:45 +0100, Hongyan Xia wrote:
> On Tue, 2020-07-14 at 14:42 +0200, Jan Beulich wrote:
> > On 29.05.2020 13:11, Hongyan Xia wrote:
> > > From: Wei Liu <wei.liu2@citrix.com>
> > > 
> > > After inspection ARM doesn't have alloc_xen_pagetable so this
> > > function
> > > is x86 only, which means it is safe for us to change.
> > 
> > Well, it sits inside a "#ifndef CONFIG_ARM" section.
> > 
> > > @@ -1442,29 +1443,42 @@ static __init void copy_mapping(unsigned
> > > long mfn, unsigned long end,
> > >                                                   unsigned long
> > > emfn))
> > >  {
> > >      unsigned long next;
> > > +    l3_pgentry_t *l3src = NULL, *l3dst = NULL;
> > >  
> > >      for ( ; mfn < end; mfn = next )
> > >      {
> > >          l4_pgentry_t l4e = efi_l4_pgtable[l4_table_offset(mfn <<
> > > PAGE_SHIFT)];
> > > -        l3_pgentry_t *l3src, *l3dst;
> > >          unsigned long va = (unsigned long)mfn_to_virt(mfn);
> > >  
> > > +        if ( !((mfn << PAGE_SHIFT) & ((1UL <<
> > > L4_PAGETABLE_SHIFT)
> > > - 1)) )
> > 
> > To be in line with ...
> > 
> > > +        {
> > > +            UNMAP_DOMAIN_PAGE(l3src);
> > > +            UNMAP_DOMAIN_PAGE(l3dst);
> > > +        }
> > >          next = mfn + (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT));
> > 
> > ... this, please avoid the left shift of mfn in the if().
> > Judgingfrom
> 
> What do you mean by "in line" here? It does not look to me that "next
> =" can be easily squashed into the if() condition.

Sorry, never mind. "in line" != "inline".

Hongyan
Jan Beulich July 27, 2020, 7:33 p.m. UTC | #4 
On 27.07.2020 14:45, Hongyan Xia wrote:
> On Tue, 2020-07-14 at 14:42 +0200, Jan Beulich wrote:
>> On 29.05.2020 13:11, Hongyan Xia wrote:
>>> @@ -1442,29 +1443,42 @@ static __init void copy_mapping(unsigned
>>> long mfn, unsigned long end,
>>>                                                    unsigned long
>>> emfn))
>>>   {
>>>       unsigned long next;
>>> +    l3_pgentry_t *l3src = NULL, *l3dst = NULL;
>>>   
>>>       for ( ; mfn < end; mfn = next )
>>>       {
>>>           l4_pgentry_t l4e = efi_l4_pgtable[l4_table_offset(mfn <<
>>> PAGE_SHIFT)];
>>> -        l3_pgentry_t *l3src, *l3dst;
>>>           unsigned long va = (unsigned long)mfn_to_virt(mfn);
>>>   
>>> +        if ( !((mfn << PAGE_SHIFT) & ((1UL << L4_PAGETABLE_SHIFT)
>>> - 1)) )
>>
>> To be in line with ...
>>
>>> +        {
>>> +            UNMAP_DOMAIN_PAGE(l3src);
>>> +            UNMAP_DOMAIN_PAGE(l3dst);
>>> +        }
>>>           next = mfn + (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT));
>>
>> ... this, please avoid the left shift of mfn in the if(). Judgingfrom
> 
> What do you mean by "in line" here? It does not look to me that "next
> =" can be easily squashed into the if() condition.

I'm not thinking about squashing anything into an if(). I've talked
about avoiding the left shift of mfn, as this last quoted line does
(by instead subtracting PAGE_SHIFT from the left-shift count.

Jan

Jan
diff mbox series

Patch

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index a6f84c945a..2599ae50d2 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -6,6 +6,7 @@ 
 #include <xen/compile.h>
 #include <xen/ctype.h>
 #include <xen/dmi.h>
+#include <xen/domain_page.h>
 #include <xen/init.h>
 #include <xen/keyhandler.h>
 #include <xen/lib.h>
@@ -1442,29 +1443,42 @@  static __init void copy_mapping(unsigned long mfn, unsigned long end,
                                                  unsigned long emfn))
 {
     unsigned long next;
+    l3_pgentry_t *l3src = NULL, *l3dst = NULL;
 
     for ( ; mfn < end; mfn = next )
     {
         l4_pgentry_t l4e = efi_l4_pgtable[l4_table_offset(mfn << PAGE_SHIFT)];
-        l3_pgentry_t *l3src, *l3dst;
         unsigned long va = (unsigned long)mfn_to_virt(mfn);
 
+        if ( !((mfn << PAGE_SHIFT) & ((1UL << L4_PAGETABLE_SHIFT) - 1)) )
+        {
+            UNMAP_DOMAIN_PAGE(l3src);
+            UNMAP_DOMAIN_PAGE(l3dst);
+        }
         next = mfn + (1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT));
         if ( !is_valid(mfn, min(next, end)) )
             continue;
-        if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
+        if ( !l3dst )
         {
-            l3dst = alloc_xen_pagetable();
-            BUG_ON(!l3dst);
-            clear_page(l3dst);
-            efi_l4_pgtable[l4_table_offset(mfn << PAGE_SHIFT)] =
-                l4e_from_paddr(virt_to_maddr(l3dst), __PAGE_HYPERVISOR);
+            if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
+            {
+                mfn_t l3mfn;
+
+                l3dst = alloc_map_clear_xen_pt(&l3mfn);
+                BUG_ON(!l3dst);
+                efi_l4_pgtable[l4_table_offset(mfn << PAGE_SHIFT)] =
+                    l4e_from_mfn(l3mfn, __PAGE_HYPERVISOR);
+            }
+            else
+                l3dst = map_l3t_from_l4e(l4e);
         }
-        else
-            l3dst = l4e_to_l3e(l4e);
-        l3src = l4e_to_l3e(idle_pg_table[l4_table_offset(va)]);
+        if ( !l3src )
+            l3src = map_l3t_from_l4e(idle_pg_table[l4_table_offset(va)]);
         l3dst[l3_table_offset(mfn << PAGE_SHIFT)] = l3src[l3_table_offset(va)];
     }
+
+    unmap_domain_page(l3src);
+    unmap_domain_page(l3dst);
 }
 
 static bool __init ram_range_valid(unsigned long smfn, unsigned long emfn)