[Xen-devel] [PATCH] p2m/ept: pass correct level to atomic_write_ept_entry in ept_invalidate_emt

[Xen-devel] [PATCH] p2m/ept: pass correct level to atomic_write_ept_entry in ept_invalidate_emt

[Roger Pau Monne]

The level passed to ept_invalidate_emt corresponds to the EPT entry
passed as the mfn parameter, which is a pointer to an EPT page table,
hence the entries in that page table will have one level less than the
parent.

Fix the call to atomic_write_ept_entry to pass the correct level, ie:
one level less than the parent.

Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: George Dunlap <george.dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
---
 xen/arch/x86/mm/p2m-ept.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index 6b8468c793..23ae6e9da3 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain *p2m, mfn_t mfn,
         e.emt = MTRR_NUM_TYPES;
         if ( recalc )
             e.recalc = 1;
-        rc = atomic_write_ept_entry(p2m, &epte[i], e, level);
+        rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1);
         ASSERT(rc == 0);
         changed = 1;
     }
-- 
2.22.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] p2m/ept: pass correct level to atomic_write_ept_entry in ept_invalidate_emt

[Tian, Kevin]

> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> Sent: Tuesday, August 20, 2019 11:38 PM
> 
> The level passed to ept_invalidate_emt corresponds to the EPT entry
> passed as the mfn parameter, which is a pointer to an EPT page table,
> hence the entries in that page table will have one level less than the
> parent.
> 
> Fix the call to atomic_write_ept_entry to pass the correct level, ie:
> one level less than the parent.
> 
> Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Jun Nakajima <jun.nakajima@intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: George Dunlap <george.dunlap@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wl@xen.org>
> Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> ---
>  xen/arch/x86/mm/p2m-ept.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> index 6b8468c793..23ae6e9da3 100644
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain
> *p2m, mfn_t mfn,
>          e.emt = MTRR_NUM_TYPES;
>          if ( recalc )
>              e.recalc = 1;
> -        rc = atomic_write_ept_entry(p2m, &epte[i], e, level);
> +        rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1);
>          ASSERT(rc == 0);
>          changed = 1;
>      }

Reviewed-by: Kevin Tian <kevin.tian@intel.com>.

One small comment about readability. What about renaming 'level'
to 'parent_level' in this function?

Thanks
Kevin
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] p2m/ept: pass correct level to atomic_write_ept_entry in ept_invalidate_emt

[Roger Pau Monné]

On Fri, Aug 23, 2019 at 05:58:29AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> > Sent: Tuesday, August 20, 2019 11:38 PM
> > 
> > The level passed to ept_invalidate_emt corresponds to the EPT entry
> > passed as the mfn parameter, which is a pointer to an EPT page table,
> > hence the entries in that page table will have one level less than the
> > parent.
> > 
> > Fix the call to atomic_write_ept_entry to pass the correct level, ie:
> > one level less than the parent.
> > 
> > Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Cc: Jun Nakajima <jun.nakajima@intel.com>
> > Cc: Kevin Tian <kevin.tian@intel.com>
> > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wl@xen.org>
> > Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
> > Cc: Paul Durrant <paul.durrant@citrix.com>
> > ---
> >  xen/arch/x86/mm/p2m-ept.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> > index 6b8468c793..23ae6e9da3 100644
> > --- a/xen/arch/x86/mm/p2m-ept.c
> > +++ b/xen/arch/x86/mm/p2m-ept.c
> > @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain
> > *p2m, mfn_t mfn,
> >          e.emt = MTRR_NUM_TYPES;
> >          if ( recalc )
> >              e.recalc = 1;
> > -        rc = atomic_write_ept_entry(p2m, &epte[i], e, level);
> > +        rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1);
> >          ASSERT(rc == 0);
> >          changed = 1;
> >      }
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>.
> 
> One small comment about readability. What about renaming 'level'
> to 'parent_level' in this function?

Sure, I guess this can be done while committing it, or else I can send
a follow up.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] p2m/ept: pass correct level to atomic_write_ept_entry in ept_invalidate_emt

[Jan Beulich]

On 23.08.2019 07:58,  Tian, Kevin  wrote:
>> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
>> Sent: Tuesday, August 20, 2019 11:38 PM
>>
>> The level passed to ept_invalidate_emt corresponds to the EPT entry
>> passed as the mfn parameter, which is a pointer to an EPT page table,
>> hence the entries in that page table will have one level less than the
>> parent.
>>
>> Fix the call to atomic_write_ept_entry to pass the correct level, ie:
>> one level less than the parent.
>>
>> Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages')
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>> Cc: Jan Beulich <jbeulich@suse.com>
>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>> Cc: Wei Liu <wl@xen.org>
>> Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
>> Cc: Paul Durrant <paul.durrant@citrix.com>
>> ---
>>   xen/arch/x86/mm/p2m-ept.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>> index 6b8468c793..23ae6e9da3 100644
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain
>> *p2m, mfn_t mfn,
>>           e.emt = MTRR_NUM_TYPES;
>>           if ( recalc )
>>               e.recalc = 1;
>> -        rc = atomic_write_ept_entry(p2m, &epte[i], e, level);
>> +        rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1);
>>           ASSERT(rc == 0);
>>           changed = 1;
>>       }
> 
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>.
> 
> One small comment about readability. What about renaming 'level'
> to 'parent_level' in this function?

And also taking the opportunity and making it unsigned int?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] p2m/ept: pass correct level to atomic_write_ept_entry in ept_invalidate_emt

[Roger Pau Monné]

On Tue, Aug 27, 2019 at 05:23:33PM +0200, Jan Beulich wrote:
> On 23.08.2019 07:58,  Tian, Kevin  wrote:
> > > From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> > > Sent: Tuesday, August 20, 2019 11:38 PM
> > > 
> > > The level passed to ept_invalidate_emt corresponds to the EPT entry
> > > passed as the mfn parameter, which is a pointer to an EPT page table,
> > > hence the entries in that page table will have one level less than the
> > > parent.
> > > 
> > > Fix the call to atomic_write_ept_entry to pass the correct level, ie:
> > > one level less than the parent.
> > > 
> > > Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages')
> > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > ---
> > > Cc: Jun Nakajima <jun.nakajima@intel.com>
> > > Cc: Kevin Tian <kevin.tian@intel.com>
> > > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> > > Cc: Jan Beulich <jbeulich@suse.com>
> > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > Cc: Wei Liu <wl@xen.org>
> > > Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
> > > Cc: Paul Durrant <paul.durrant@citrix.com>
> > > ---
> > >   xen/arch/x86/mm/p2m-ept.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> > > index 6b8468c793..23ae6e9da3 100644
> > > --- a/xen/arch/x86/mm/p2m-ept.c
> > > +++ b/xen/arch/x86/mm/p2m-ept.c
> > > @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain
> > > *p2m, mfn_t mfn,
> > >           e.emt = MTRR_NUM_TYPES;
> > >           if ( recalc )
> > >               e.recalc = 1;
> > > -        rc = atomic_write_ept_entry(p2m, &epte[i], e, level);
> > > +        rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1);
> > >           ASSERT(rc == 0);
> > >           changed = 1;
> > >       }
> > 
> > Reviewed-by: Kevin Tian <kevin.tian@intel.com>.
> > 
> > One small comment about readability. What about renaming 'level'
> > to 'parent_level' in this function?
> 
> And also taking the opportunity and making it unsigned int?

I've been thinking about this, and I'm not sure renaming level to
parent_level is correct, level actually matches the level of the mfn
also passed as a parameter, and hence it's correct. It's the function
itself that actually iterates over the page table entries, and hence
descends one level into the page tables.

ie: I could add something like ASSERT(level) to make sure no level 0
entries are passed to the function, but renaming doesn't seem correct
to me.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] p2m/ept: pass correct level to atomic_write_ept_entry in ept_invalidate_emt

[Jan Beulich]

On 29.08.2019 12:26, Roger Pau Monné  wrote:
> On Tue, Aug 27, 2019 at 05:23:33PM +0200, Jan Beulich wrote:
>> On 23.08.2019 07:58,  Tian, Kevin  wrote:
>>>> From: Roger Pau Monne [mailto:roger.pau@citrix.com]
>>>> Sent: Tuesday, August 20, 2019 11:38 PM
>>>>
>>>> The level passed to ept_invalidate_emt corresponds to the EPT entry
>>>> passed as the mfn parameter, which is a pointer to an EPT page table,
>>>> hence the entries in that page table will have one level less than the
>>>> parent.
>>>>
>>>> Fix the call to atomic_write_ept_entry to pass the correct level, ie:
>>>> one level less than the parent.
>>>>
>>>> Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages')
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> ---
>>>> Cc: Jun Nakajima <jun.nakajima@intel.com>
>>>> Cc: Kevin Tian <kevin.tian@intel.com>
>>>> Cc: George Dunlap <george.dunlap@eu.citrix.com>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Cc: Wei Liu <wl@xen.org>
>>>> Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
>>>> Cc: Paul Durrant <paul.durrant@citrix.com>
>>>> ---
>>>>   xen/arch/x86/mm/p2m-ept.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>>>> index 6b8468c793..23ae6e9da3 100644
>>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>>> @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct p2m_domain
>>>> *p2m, mfn_t mfn,
>>>>           e.emt = MTRR_NUM_TYPES;
>>>>           if ( recalc )
>>>>               e.recalc = 1;
>>>> -        rc = atomic_write_ept_entry(p2m, &epte[i], e, level);
>>>> +        rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1);
>>>>           ASSERT(rc == 0);
>>>>           changed = 1;
>>>>       }
>>>
>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>.
>>>
>>> One small comment about readability. What about renaming 'level'
>>> to 'parent_level' in this function?
>>
>> And also taking the opportunity and making it unsigned int?
> 
> I've been thinking about this, and I'm not sure renaming level to
> parent_level is correct, level actually matches the level of the mfn
> also passed as a parameter, and hence it's correct. It's the function
> itself that actually iterates over the page table entries, and hence
> descends one level into the page tables.
> 
> ie: I could add something like ASSERT(level) to make sure no level 0
> entries are passed to the function, but renaming doesn't seem correct
> to me.

Hmm, I'm afraid I've made the change as requested by Kevin while
committing. Personally I think either name is fine, but if Kevin
agrees with your response, then maybe we should undo that adjustment.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] p2m/ept: pass correct level to atomic_write_ept_entry in ept_invalidate_emt

[Tian, Kevin]

> From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> Sent: Thursday, August 29, 2019 6:26 PM
> 
> On Tue, Aug 27, 2019 at 05:23:33PM +0200, Jan Beulich wrote:
> > On 23.08.2019 07:58,  Tian, Kevin  wrote:
> > > > From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> > > > Sent: Tuesday, August 20, 2019 11:38 PM
> > > >
> > > > The level passed to ept_invalidate_emt corresponds to the EPT entry
> > > > passed as the mfn parameter, which is a pointer to an EPT page table,
> > > > hence the entries in that page table will have one level less than the
> > > > parent.
> > > >
> > > > Fix the call to atomic_write_ept_entry to pass the correct level, ie:
> > > > one level less than the parent.
> > > >
> > > > Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages')
> > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > ---
> > > > Cc: Jun Nakajima <jun.nakajima@intel.com>
> > > > Cc: Kevin Tian <kevin.tian@intel.com>
> > > > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> > > > Cc: Jan Beulich <jbeulich@suse.com>
> > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > Cc: Wei Liu <wl@xen.org>
> > > > Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
> > > > Cc: Paul Durrant <paul.durrant@citrix.com>
> > > > ---
> > > >   xen/arch/x86/mm/p2m-ept.c | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> > > > index 6b8468c793..23ae6e9da3 100644
> > > > --- a/xen/arch/x86/mm/p2m-ept.c
> > > > +++ b/xen/arch/x86/mm/p2m-ept.c
> > > > @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct
> p2m_domain
> > > > *p2m, mfn_t mfn,
> > > >           e.emt = MTRR_NUM_TYPES;
> > > >           if ( recalc )
> > > >               e.recalc = 1;
> > > > -        rc = atomic_write_ept_entry(p2m, &epte[i], e, level);
> > > > +        rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1);
> > > >           ASSERT(rc == 0);
> > > >           changed = 1;
> > > >       }
> > >
> > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>.
> > >
> > > One small comment about readability. What about renaming 'level'
> > > to 'parent_level' in this function?
> >
> > And also taking the opportunity and making it unsigned int?
> 
> I've been thinking about this, and I'm not sure renaming level to
> parent_level is correct, level actually matches the level of the mfn
> also passed as a parameter, and hence it's correct. It's the function
> itself that actually iterates over the page table entries, and hence
> descends one level into the page tables.
> 
> ie: I could add something like ASSERT(level) to make sure no level 0
> entries are passed to the function, but renaming doesn't seem correct
> to me.
> 

The problem to me is that it is very obscure about what a 'level' actually
means. Although I figured it out last time when reviewing this patch,
now I'm getting confused again when looking at related code. e.g., you
minus level by one in this patch when invoking atomic_write_ept_entry,
while the latter increments the level by one when invoking p2m_entry_
modify. They are all about entry update according to the function name,
but clearly the level means different thing. :/

specifically for this patch, maybe call ept_invalidate_emt_subtree can
also improve readability a bit, along with ASSERT(level) thing?

Thanks
Kevin

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH] p2m/ept: pass correct level to atomic_write_ept_entry in ept_invalidate_emt

[Roger Pau Monné]

On Wed, Sep 04, 2019 at 06:50:25AM +0000, Tian, Kevin wrote:
> > From: Roger Pau Monné [mailto:roger.pau@citrix.com]
> > Sent: Thursday, August 29, 2019 6:26 PM
> > 
> > On Tue, Aug 27, 2019 at 05:23:33PM +0200, Jan Beulich wrote:
> > > On 23.08.2019 07:58,  Tian, Kevin  wrote:
> > > > > From: Roger Pau Monne [mailto:roger.pau@citrix.com]
> > > > > Sent: Tuesday, August 20, 2019 11:38 PM
> > > > >
> > > > > The level passed to ept_invalidate_emt corresponds to the EPT entry
> > > > > passed as the mfn parameter, which is a pointer to an EPT page table,
> > > > > hence the entries in that page table will have one level less than the
> > > > > parent.
> > > > >
> > > > > Fix the call to atomic_write_ept_entry to pass the correct level, ie:
> > > > > one level less than the parent.
> > > > >
> > > > > Fixes: 50fe6e73705 ('pvh dom0: add and remove foreign pages')
> > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > > > > ---
> > > > > Cc: Jun Nakajima <jun.nakajima@intel.com>
> > > > > Cc: Kevin Tian <kevin.tian@intel.com>
> > > > > Cc: George Dunlap <george.dunlap@eu.citrix.com>
> > > > > Cc: Jan Beulich <jbeulich@suse.com>
> > > > > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > > > > Cc: Wei Liu <wl@xen.org>
> > > > > Cc: Sergey Dyasli <sergey.dyasli@citrix.com>
> > > > > Cc: Paul Durrant <paul.durrant@citrix.com>
> > > > > ---
> > > > >   xen/arch/x86/mm/p2m-ept.c | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
> > > > > index 6b8468c793..23ae6e9da3 100644
> > > > > --- a/xen/arch/x86/mm/p2m-ept.c
> > > > > +++ b/xen/arch/x86/mm/p2m-ept.c
> > > > > @@ -367,7 +367,7 @@ static bool_t ept_invalidate_emt(struct
> > p2m_domain
> > > > > *p2m, mfn_t mfn,
> > > > >           e.emt = MTRR_NUM_TYPES;
> > > > >           if ( recalc )
> > > > >               e.recalc = 1;
> > > > > -        rc = atomic_write_ept_entry(p2m, &epte[i], e, level);
> > > > > +        rc = atomic_write_ept_entry(p2m, &epte[i], e, level - 1);
> > > > >           ASSERT(rc == 0);
> > > > >           changed = 1;
> > > > >       }
> > > >
> > > > Reviewed-by: Kevin Tian <kevin.tian@intel.com>.
> > > >
> > > > One small comment about readability. What about renaming 'level'
> > > > to 'parent_level' in this function?
> > >
> > > And also taking the opportunity and making it unsigned int?
> > 
> > I've been thinking about this, and I'm not sure renaming level to
> > parent_level is correct, level actually matches the level of the mfn
> > also passed as a parameter, and hence it's correct. It's the function
> > itself that actually iterates over the page table entries, and hence
> > descends one level into the page tables.
> > 
> > ie: I could add something like ASSERT(level) to make sure no level 0
> > entries are passed to the function, but renaming doesn't seem correct
> > to me.
> > 
> 
> The problem to me is that it is very obscure about what a 'level' actually
> means. Although I figured it out last time when reviewing this patch,
> now I'm getting confused again when looking at related code. e.g., you
> minus level by one in this patch when invoking atomic_write_ept_entry,

That minus one is because the level of EPT entry in the
atomic_write_ept_entry call is one level less than the parent, which
is the parameter of the function.

> while the latter increments the level by one when invoking p2m_entry_
> modify. They are all about entry update according to the function name,
> but clearly the level means different thing. :/

That's unfortunate, but NPT/shadow considers that level starts at 1
(ie: 4K page-table entries are level 1), while EPT consider that level
starts at 0 (ie: 4K page-table entries are level 0). That's IMO a very
bad choice, I guess no one realized before of this difference
because level was always internal to NPT or EPT code, but no generic
functions using such level parameter where being called from both
implementations.

> specifically for this patch, maybe call ept_invalidate_emt_subtree can
> also improve readability a bit, along with ASSERT(level) thing?

I agree, let me try to prepare a patch.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel