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 <jbeul...@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
