For non-present entries EMT, like most other fields, is meaningless to
hardware. Make the logic in ept_set_entry() setting the field (and iPAT)
conditional upon dealing with a present entry, leaving the value at 0
otherwise. This has two effects for epte_get_entry_emt() which we'll
want to leverage subsequently:
1) The call moved here now won't be issued with INVALID_MFN anymore (a
   respective BUG_ON() is being added).
2) Neither of the other two calls could now be issued with a truncated
   form of INVALID_MFN anymore (as long as there's no bug anywhere
   marking an entry present when that was populated using INVALID_MFN).

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

--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -650,6 +650,8 @@ static int cf_check resolve_misconfig(st
             if ( e.emt != MTRR_NUM_TYPES )
                 break;
 
+            ASSERT(is_epte_present(&e));
+
             if ( level == 0 )
             {
                 for ( gfn -= i, i = 0; i < EPT_PAGETABLE_ENTRIES; ++i )
@@ -915,17 +917,6 @@ ept_set_entry(struct p2m_domain *p2m, gf
 
     if ( mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt) )
     {
-        bool ipat;
-        int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn), mfn,
-                                     i * EPT_TABLE_ORDER, &ipat,
-                                     p2mt);
-
-        if ( emt >= 0 )
-            new_entry.emt = emt;
-        else /* ept_handle_misconfig() will need to take care of this. */
-            new_entry.emt = MTRR_NUM_TYPES;
-
-        new_entry.ipat = ipat;
         new_entry.sp = !!i;
         new_entry.sa_p2mt = p2mt;
         new_entry.access = p2ma;
@@ -941,6 +932,22 @@ ept_set_entry(struct p2m_domain *p2m, gf
             need_modify_vtd_table = 0;
 
         ept_p2m_type_to_flags(p2m, &new_entry);
+
+        if ( is_epte_present(&new_entry) )
+        {
+            bool ipat;
+            int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn), mfn,
+                                         i * EPT_TABLE_ORDER, &ipat,
+                                         p2mt);
+
+            BUG_ON(mfn_eq(mfn, INVALID_MFN));
+
+            if ( emt >= 0 )
+                new_entry.emt = emt;
+            else /* ept_handle_misconfig() will need to take care of this. */
+                new_entry.emt = MTRR_NUM_TYPES;
+            new_entry.ipat = ipat;
+        }
     }
 
     if ( sve != -1 )

On Wed, 2024-06-12 at 15:16 +0200, Jan Beulich wrote:
> For non-present entries EMT, like most other fields, is meaningless
> to
> hardware. Make the logic in ept_set_entry() setting the field (and
> iPAT)
> conditional upon dealing with a present entry, leaving the value at 0
> otherwise. This has two effects for epte_get_entry_emt() which we'll
> want to leverage subsequently:
> 1) The call moved here now won't be issued with INVALID_MFN anymore
> (a
>    respective BUG_ON() is being added).
> 2) Neither of the other two calls could now be issued with a
> truncated
>    form of INVALID_MFN anymore (as long as there's no bug anywhere
>    marking an entry present when that was populated using
> INVALID_MFN).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Release-Acked-By: Oleksii Kurochko <oleksii.kurochko@gmail.com>

~ Oleksii
> ---
> v2: New.
> 
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -650,6 +650,8 @@ static int cf_check resolve_misconfig(st
>              if ( e.emt != MTRR_NUM_TYPES )
>                  break;
>  
> +            ASSERT(is_epte_present(&e));
> +
>              if ( level == 0 )
>              {
>                  for ( gfn -= i, i = 0; i < EPT_PAGETABLE_ENTRIES;
> ++i )
> @@ -915,17 +917,6 @@ ept_set_entry(struct p2m_domain *p2m, gf
>  
>      if ( mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt) )
>      {
> -        bool ipat;
> -        int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn), mfn,
> -                                     i * EPT_TABLE_ORDER, &ipat,
> -                                     p2mt);
> -
> -        if ( emt >= 0 )
> -            new_entry.emt = emt;
> -        else /* ept_handle_misconfig() will need to take care of
> this. */
> -            new_entry.emt = MTRR_NUM_TYPES;
> -
> -        new_entry.ipat = ipat;
>          new_entry.sp = !!i;
>          new_entry.sa_p2mt = p2mt;
>          new_entry.access = p2ma;
> @@ -941,6 +932,22 @@ ept_set_entry(struct p2m_domain *p2m, gf
>              need_modify_vtd_table = 0;
>  
>          ept_p2m_type_to_flags(p2m, &new_entry);
> +
> +        if ( is_epte_present(&new_entry) )
> +        {
> +            bool ipat;
> +            int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn),
> mfn,
> +                                         i * EPT_TABLE_ORDER, &ipat,
> +                                         p2mt);
> +
> +            BUG_ON(mfn_eq(mfn, INVALID_MFN));
> +
> +            if ( emt >= 0 )
> +                new_entry.emt = emt;
> +            else /* ept_handle_misconfig() will need to take care of
> this. */
> +                new_entry.emt = MTRR_NUM_TYPES;
> +            new_entry.ipat = ipat;
> +        }
>      }
>  
>      if ( sve != -1 )
> 

On Wed, Jun 12, 2024 at 03:16:59PM +0200, Jan Beulich wrote:
> For non-present entries EMT, like most other fields, is meaningless to
> hardware. Make the logic in ept_set_entry() setting the field (and iPAT)
> conditional upon dealing with a present entry, leaving the value at 0
> otherwise. This has two effects for epte_get_entry_emt() which we'll
> want to leverage subsequently:
> 1) The call moved here now won't be issued with INVALID_MFN anymore (a
>    respective BUG_ON() is being added).
> 2) Neither of the other two calls could now be issued with a truncated
>    form of INVALID_MFN anymore (as long as there's no bug anywhere
>    marking an entry present when that was populated using INVALID_MFN).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: New.
> 
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -650,6 +650,8 @@ static int cf_check resolve_misconfig(st
>              if ( e.emt != MTRR_NUM_TYPES )
>                  break;
>  
> +            ASSERT(is_epte_present(&e));

If this is added here, then there's a condition further below:

if ( !is_epte_valid(&e) || !is_epte_present(&e) )

That needs adjusting AFAICT.

However, in ept_set_entry() we seem to unconditionally call
resolve_misconfig() against the new entry to be populated, won't this
possibly cause resolve_misconfig() to be called against non-present
EPT entries?  I think this is fine because such non-present entries
will have emt == 0, and hence will take the break just ahead of the
added ASSERT().

> +
>              if ( level == 0 )
>              {
>                  for ( gfn -= i, i = 0; i < EPT_PAGETABLE_ENTRIES; ++i )
> @@ -915,17 +917,6 @@ ept_set_entry(struct p2m_domain *p2m, gf
>  
>      if ( mfn_valid(mfn) || p2m_allows_invalid_mfn(p2mt) )
>      {
> -        bool ipat;
> -        int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn), mfn,
> -                                     i * EPT_TABLE_ORDER, &ipat,
> -                                     p2mt);
> -
> -        if ( emt >= 0 )
> -            new_entry.emt = emt;
> -        else /* ept_handle_misconfig() will need to take care of this. */
> -            new_entry.emt = MTRR_NUM_TYPES;
> -
> -        new_entry.ipat = ipat;
>          new_entry.sp = !!i;
>          new_entry.sa_p2mt = p2mt;
>          new_entry.access = p2ma;
> @@ -941,6 +932,22 @@ ept_set_entry(struct p2m_domain *p2m, gf
>              need_modify_vtd_table = 0;
>  
>          ept_p2m_type_to_flags(p2m, &new_entry);
> +
> +        if ( is_epte_present(&new_entry) )
> +        {
> +            bool ipat;
> +            int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn), mfn,
> +                                         i * EPT_TABLE_ORDER, &ipat,
> +                                         p2mt);
> +
> +            BUG_ON(mfn_eq(mfn, INVALID_MFN));
> +
> +            if ( emt >= 0 )
> +                new_entry.emt = emt;
> +            else /* ept_handle_misconfig() will need to take care of this. */
> +                new_entry.emt = MTRR_NUM_TYPES;
> +            new_entry.ipat = ipat;
> +        }

Should we assert that if new_entry.emt == MTRR_NUM_TYPES the entry
must have the present bit set before the atomic_write_ept_entry()
call?

Thanks, Roger.

On 12.06.2024 16:38, Roger Pau Monné wrote:
> On Wed, Jun 12, 2024 at 03:16:59PM +0200, Jan Beulich wrote:
>> For non-present entries EMT, like most other fields, is meaningless to
>> hardware. Make the logic in ept_set_entry() setting the field (and iPAT)
>> conditional upon dealing with a present entry, leaving the value at 0
>> otherwise. This has two effects for epte_get_entry_emt() which we'll
>> want to leverage subsequently:
>> 1) The call moved here now won't be issued with INVALID_MFN anymore (a
>>    respective BUG_ON() is being added).
>> 2) Neither of the other two calls could now be issued with a truncated
>>    form of INVALID_MFN anymore (as long as there's no bug anywhere
>>    marking an entry present when that was populated using INVALID_MFN).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: New.
>>
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -650,6 +650,8 @@ static int cf_check resolve_misconfig(st
>>              if ( e.emt != MTRR_NUM_TYPES )
>>                  break;
>>  
>> +            ASSERT(is_epte_present(&e));
> 
> If this is added here, then there's a condition further below:
> 
> if ( !is_epte_valid(&e) || !is_epte_present(&e) )
> 
> That needs adjusting AFAICT.

I don't think so, because e was re-fetched in between.

> However, in ept_set_entry() we seem to unconditionally call
> resolve_misconfig() against the new entry to be populated, won't this
> possibly cause resolve_misconfig() to be called against non-present
> EPT entries?  I think this is fine because such non-present entries
> will have emt == 0, and hence will take the break just ahead of the
> added ASSERT().

Right, hence how I placed this assertion.

>> @@ -941,6 +932,22 @@ ept_set_entry(struct p2m_domain *p2m, gf
>>              need_modify_vtd_table = 0;
>>  
>>          ept_p2m_type_to_flags(p2m, &new_entry);
>> +
>> +        if ( is_epte_present(&new_entry) )
>> +        {
>> +            bool ipat;
>> +            int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn), mfn,
>> +                                         i * EPT_TABLE_ORDER, &ipat,
>> +                                         p2mt);
>> +
>> +            BUG_ON(mfn_eq(mfn, INVALID_MFN));
>> +
>> +            if ( emt >= 0 )
>> +                new_entry.emt = emt;
>> +            else /* ept_handle_misconfig() will need to take care of this. */
>> +                new_entry.emt = MTRR_NUM_TYPES;
>> +            new_entry.ipat = ipat;
>> +        }
> 
> Should we assert that if new_entry.emt == MTRR_NUM_TYPES the entry
> must have the present bit set before the atomic_write_ept_entry()
> call?

This would feel excessive to me. All writing to new_entry is close together,
immediately ahead of that atomic_write_ept_entry(). And we're (now) writing
MTRR_NUM_TYPES only when is_epte_present() is true (note that it's not "the
present bit").

Jan

On Wed, Jun 12, 2024 at 04:53:14PM +0200, Jan Beulich wrote:
> On 12.06.2024 16:38, Roger Pau Monné wrote:
> > On Wed, Jun 12, 2024 at 03:16:59PM +0200, Jan Beulich wrote:
> >> For non-present entries EMT, like most other fields, is meaningless to
> >> hardware. Make the logic in ept_set_entry() setting the field (and iPAT)
> >> conditional upon dealing with a present entry, leaving the value at 0
> >> otherwise. This has two effects for epte_get_entry_emt() which we'll
> >> want to leverage subsequently:
> >> 1) The call moved here now won't be issued with INVALID_MFN anymore (a
> >>    respective BUG_ON() is being added).
> >> 2) Neither of the other two calls could now be issued with a truncated
> >>    form of INVALID_MFN anymore (as long as there's no bug anywhere
> >>    marking an entry present when that was populated using INVALID_MFN).
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> >> ---
> >> v2: New.
> >>
> >> --- a/xen/arch/x86/mm/p2m-ept.c
> >> +++ b/xen/arch/x86/mm/p2m-ept.c
> >> @@ -650,6 +650,8 @@ static int cf_check resolve_misconfig(st
> >>              if ( e.emt != MTRR_NUM_TYPES )
> >>                  break;
> >>  
> >> +            ASSERT(is_epte_present(&e));
> > 
> > If this is added here, then there's a condition further below:
> > 
> > if ( !is_epte_valid(&e) || !is_epte_present(&e) )
> > 
> > That needs adjusting AFAICT.
> 
> I don't think so, because e was re-fetched in between.

Oh, I see, we take the opportunity to do the recalculation for all the
EPT entries that share the same page table.

> > However, in ept_set_entry() we seem to unconditionally call
> > resolve_misconfig() against the new entry to be populated, won't this
> > possibly cause resolve_misconfig() to be called against non-present
> > EPT entries?  I think this is fine because such non-present entries
> > will have emt == 0, and hence will take the break just ahead of the
> > added ASSERT().
> 
> Right, hence how I placed this assertion.

OK, just wanted to double check.

> >> @@ -941,6 +932,22 @@ ept_set_entry(struct p2m_domain *p2m, gf
> >>              need_modify_vtd_table = 0;
> >>  
> >>          ept_p2m_type_to_flags(p2m, &new_entry);
> >> +
> >> +        if ( is_epte_present(&new_entry) )
> >> +        {
> >> +            bool ipat;
> >> +            int emt = epte_get_entry_emt(p2m->domain, _gfn(gfn), mfn,
> >> +                                         i * EPT_TABLE_ORDER, &ipat,
> >> +                                         p2mt);
> >> +
> >> +            BUG_ON(mfn_eq(mfn, INVALID_MFN));
> >> +
> >> +            if ( emt >= 0 )
> >> +                new_entry.emt = emt;
> >> +            else /* ept_handle_misconfig() will need to take care of this. */
> >> +                new_entry.emt = MTRR_NUM_TYPES;
> >> +            new_entry.ipat = ipat;
> >> +        }
> > 
> > Should we assert that if new_entry.emt == MTRR_NUM_TYPES the entry
> > must have the present bit set before the atomic_write_ept_entry()
> > call?
> 
> This would feel excessive to me. All writing to new_entry is close together,
> immediately ahead of that atomic_write_ept_entry(). And we're (now) writing
> MTRR_NUM_TYPES only when is_epte_present() is true (note that it's not "the
> present bit").

Fair enough.

Thanks, Roger.