diff mbox series

[1/6] x86_64/mm: map and unmap page tables in cleanup_frame_table

Message ID 12c4fe0c0c05b9f76377c085d8a6558beae64003.1587116799.git.hongyxia@amazon.com (mailing list archive)
State New, archived
Headers show
Series convert more Xen page table code to the new API | expand

Commit Message

Hongyan Xia April 17, 2020, 9:52 a.m. UTC 
From: Wei Liu <wei.liu2@citrix.com>

Also fix a weird indentation.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
---
 xen/arch/x86/x86_64/mm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Julien Grall April 24, 2020, 8:58 a.m. UTC | #1 
Hi,

On 17/04/2020 10:52, Hongyan Xia wrote:> diff --git 
a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c> index 
e85ef449f3..18210405f4 100644> --- a/xen/arch/x86/x86_64/mm.c> +++ 
b/xen/arch/x86/x86_64/mm.c> @@ -737,8 +737,8 @@ static void 
cleanup_frame_table(struct mem_hotadd_info *info)>   >       while (sva 
< eva)>       {> -        l3e = 
l4e_to_l3e(idle_pg_table[l4_table_offset(sva)])[> - 
l3_table_offset(sva)];> +        l3e = 
l3e_from_l4e(idle_pg_table[l4_table_offset(sva)],> + 
       l3_table_offset(sva));
This macro doesn't exist yet in the tree. It would be good to spell out 
the dependencies in the cover letter so this doesn't get merged before 
the dependency is merged.

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,
Julien Grall April 24, 2020, 8:59 a.m. UTC | #2 
On 24/04/2020 09:58, Julien Grall wrote:
> Hi,
> 
> On 17/04/2020 10:52, Hongyan Xia wrote:> diff --git 
> a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c> index 
> e85ef449f3..18210405f4 100644> --- a/xen/arch/x86/x86_64/mm.c> +++ 
> b/xen/arch/x86/x86_64/mm.c> @@ -737,8 +737,8 @@ static void 
> cleanup_frame_table(struct mem_hotadd_info *info)>   >       while (sva 
> < eva)>       {> -        l3e = 
> l4e_to_l3e(idle_pg_table[l4_table_offset(sva)])[> - 
> l3_table_offset(sva)];> +        l3e = 
> l3e_from_l4e(idle_pg_table[l4_table_offset(sva)],> +       
> l3_table_offset(sva));
> This macro doesn't exist yet in the tree. It would be good to spell out 
> the dependencies in the cover letter so this doesn't get merged before 
> the dependency is merged.
> 
> Reviewed-by: Julien Grall <jgrall@amazon.com>

Argh, I screwed the reply. Sorry for that. I will resend it.
Julien Grall April 24, 2020, 8:59 a.m. UTC | #3 
(resending)

On 17/04/2020 10:52, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
> 
> Also fix a weird indentation.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
> ---
>   xen/arch/x86/x86_64/mm.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index e85ef449f3..18210405f4 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -737,8 +737,8 @@ static void cleanup_frame_table(struct mem_hotadd_info *info)
>   
>       while (sva < eva)
>       {
> -        l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(sva)])[
> -          l3_table_offset(sva)];
> +        l3e = l3e_from_l4e(idle_pg_table[l4_table_offset(sva)],
> +                           l3_table_offset(sva));

This macro doesn't exist yet in the tree. It would be good to spell out 
the dependencies in the cover letter so this doesn't get merged before 
the dependency is merged.

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,
Julien Grall April 24, 2020, 9:02 a.m. UTC | #4 
On 17/04/2020 10:52, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@citrix.com>
> 
> Also fix a weird indentation.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
> ---
>   xen/arch/x86/x86_64/mm.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index e85ef449f3..18210405f4 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -737,8 +737,8 @@ static void cleanup_frame_table(struct mem_hotadd_info *info)
>   
>       while (sva < eva)
>       {
> -        l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(sva)])[
> -          l3_table_offset(sva)];
> +        l3e = l3e_from_l4e(idle_pg_table[l4_table_offset(sva)],
> +                           l3_table_offset(sva));
>           if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) ||
>                (l3e_get_flags(l3e) & _PAGE_PSE) )
>           {
> @@ -747,7 +747,7 @@ static void cleanup_frame_table(struct mem_hotadd_info *info)
>               continue;
>           }
>   
> -        l2e = l3e_to_l2e(l3e)[l2_table_offset(sva)];
> +        l2e = l2e_from_l3e(l3e, l2_table_offset(sva));
>           ASSERT(l2e_get_flags(l2e) & _PAGE_PRESENT);
>   
>           if ( (l2e_get_flags(l2e) & (_PAGE_PRESENT | _PAGE_PSE)) ==
> @@ -763,10 +763,10 @@ static void cleanup_frame_table(struct mem_hotadd_info *info)
>               continue;
>           }
>   
> -        ASSERT(l1e_get_flags(l2e_to_l1e(l2e)[l1_table_offset(sva)]) &
> -                _PAGE_PRESENT);
> -         sva = (sva & ~((1UL << PAGE_SHIFT) - 1)) +
> -                    (1UL << PAGE_SHIFT);
> +        ASSERT(l1e_get_flags(l1e_from_l2e(l2e, l1_table_offset(sva))) &
> +               _PAGE_PRESENT);
> +
> +        sva = (sva & ~((1UL << PAGE_SHIFT) - 1)) + (1UL << PAGE_SHIFT);

NIT: While you are modifying the indentation. Couldn't we use PAGE_MASK 
and PAGE_SIZE here?

Cheers,
Hongyan Xia April 24, 2020, 9:21 a.m. UTC | #5 
Hi Julien,

On Fri, 2020-04-24 at 09:59 +0100, Julien Grall wrote:
> (resending)
> 
> On 17/04/2020 10:52, Hongyan Xia wrote:
> > From: Wei Liu <wei.liu2@citrix.com>
> > 
> > Also fix a weird indentation.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
> > ---
> >   xen/arch/x86/x86_64/mm.c | 14 +++++++-------
> >   1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> > index e85ef449f3..18210405f4 100644
> > --- a/xen/arch/x86/x86_64/mm.c
> > +++ b/xen/arch/x86/x86_64/mm.c
> > @@ -737,8 +737,8 @@ static void cleanup_frame_table(struct
> > mem_hotadd_info *info)
> >   
> >       while (sva < eva)
> >       {
> > -        l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(sva)])[
> > -          l3_table_offset(sva)];
> > +        l3e = l3e_from_l4e(idle_pg_table[l4_table_offset(sva)],
> > +                           l3_table_offset(sva));
> 
> This macro doesn't exist yet in the tree. It would be good to spell
> out 
> the dependencies in the cover letter so this doesn't get merged
> before 
> the dependency is merged.

I believe the introduction of the new macros has been merged in staging
as 6c8afe5aadb33761431b24157d99b25eac15fc7e.

> Reviewed-by: Julien Grall <jgrall@amazon.com>

Thanks!

Hongyan
Julien Grall April 24, 2020, 9:24 a.m. UTC | #6 
On 24/04/2020 10:21, Hongyan Xia wrote:
> Hi Julien,
> 
> On Fri, 2020-04-24 at 09:59 +0100, Julien Grall wrote:
>> (resending)
>>
>> On 17/04/2020 10:52, Hongyan Xia wrote:
>>> From: Wei Liu <wei.liu2@citrix.com>
>>>
>>> Also fix a weird indentation.
>>>
>>> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
>>> Signed-off-by: Hongyan Xia <hongyxia@amazon.com>
>>> ---
>>>    xen/arch/x86/x86_64/mm.c | 14 +++++++-------
>>>    1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
>>> index e85ef449f3..18210405f4 100644
>>> --- a/xen/arch/x86/x86_64/mm.c
>>> +++ b/xen/arch/x86/x86_64/mm.c
>>> @@ -737,8 +737,8 @@ static void cleanup_frame_table(struct
>>> mem_hotadd_info *info)
>>>    
>>>        while (sva < eva)
>>>        {
>>> -        l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(sva)])[
>>> -          l3_table_offset(sva)];
>>> +        l3e = l3e_from_l4e(idle_pg_table[l4_table_offset(sva)],
>>> +                           l3_table_offset(sva));
>>
>> This macro doesn't exist yet in the tree. It would be good to spell
>> out
>> the dependencies in the cover letter so this doesn't get merged
>> before
>> the dependency is merged.
> 
> I believe the introduction of the new macros has been merged in staging
> as 6c8afe5aadb33761431b24157d99b25eac15fc7e.

Hmmmm you are right. I must have been blind. Sorry for the noise.

Cheers,
Jan Beulich April 24, 2020, 11:12 a.m. UTC | #7 
On 24.04.2020 11:02, Julien Grall wrote:
> On 17/04/2020 10:52, Hongyan Xia wrote:
>> @@ -763,10 +763,10 @@ static void cleanup_frame_table(struct mem_hotadd_info *info)
>>               continue;
>>           }
>>   -        ASSERT(l1e_get_flags(l2e_to_l1e(l2e)[l1_table_offset(sva)]) &
>> -                _PAGE_PRESENT);
>> -         sva = (sva & ~((1UL << PAGE_SHIFT) - 1)) +
>> -                    (1UL << PAGE_SHIFT);
>> +        ASSERT(l1e_get_flags(l1e_from_l2e(l2e, l1_table_offset(sva))) &
>> +               _PAGE_PRESENT);
>> +
>> +        sva = (sva & ~((1UL << PAGE_SHIFT) - 1)) + (1UL << PAGE_SHIFT);
> 
> NIT: While you are modifying the indentation. Couldn't we use PAGE_MASK and PAGE_SIZE here?

Oh, yes, this would be nice.

Jan
Jan Beulich April 28, 2020, 3:11 p.m. UTC | #8 
On 24.04.2020 11:02, Julien Grall wrote:
> On 17/04/2020 10:52, Hongyan Xia wrote:
>> @@ -763,10 +763,10 @@ static void cleanup_frame_table(struct mem_hotadd_info *info)
>>               continue;
>>           }
>>   -        ASSERT(l1e_get_flags(l2e_to_l1e(l2e)[l1_table_offset(sva)]) &
>> -                _PAGE_PRESENT);
>> -         sva = (sva & ~((1UL << PAGE_SHIFT) - 1)) +
>> -                    (1UL << PAGE_SHIFT);
>> +        ASSERT(l1e_get_flags(l1e_from_l2e(l2e, l1_table_offset(sva))) &
>> +               _PAGE_PRESENT);
>> +
>> +        sva = (sva & ~((1UL << PAGE_SHIFT) - 1)) + (1UL << PAGE_SHIFT);
> 
> NIT: While you are modifying the indentation. Couldn't we use PAGE_MASK and PAGE_SIZE here?

And with this (which I think can be done while committing)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index e85ef449f3..18210405f4 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -737,8 +737,8 @@  static void cleanup_frame_table(struct mem_hotadd_info *info)
 
     while (sva < eva)
     {
-        l3e = l4e_to_l3e(idle_pg_table[l4_table_offset(sva)])[
-          l3_table_offset(sva)];
+        l3e = l3e_from_l4e(idle_pg_table[l4_table_offset(sva)],
+                           l3_table_offset(sva));
         if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) ||
              (l3e_get_flags(l3e) & _PAGE_PSE) )
         {
@@ -747,7 +747,7 @@  static void cleanup_frame_table(struct mem_hotadd_info *info)
             continue;
         }
 
-        l2e = l3e_to_l2e(l3e)[l2_table_offset(sva)];
+        l2e = l2e_from_l3e(l3e, l2_table_offset(sva));
         ASSERT(l2e_get_flags(l2e) & _PAGE_PRESENT);
 
         if ( (l2e_get_flags(l2e) & (_PAGE_PRESENT | _PAGE_PSE)) ==
@@ -763,10 +763,10 @@  static void cleanup_frame_table(struct mem_hotadd_info *info)
             continue;
         }
 
-        ASSERT(l1e_get_flags(l2e_to_l1e(l2e)[l1_table_offset(sva)]) &
-                _PAGE_PRESENT);
-         sva = (sva & ~((1UL << PAGE_SHIFT) - 1)) +
-                    (1UL << PAGE_SHIFT);
+        ASSERT(l1e_get_flags(l1e_from_l2e(l2e, l1_table_offset(sva))) &
+               _PAGE_PRESENT);
+
+        sva = (sva & ~((1UL << PAGE_SHIFT) - 1)) + (1UL << PAGE_SHIFT);
     }
 
     /* Brute-Force flush all TLB */