Re: [PATCH v2] x86/PoD: defer nested P2M flushes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.10.2021 15:53, Roger Pau Monné wrote:
> On Tue, Oct 19, 2021 at 02:52:27PM +0200, Jan Beulich wrote:
>> With NPT or shadow in use, the p2m_set_entry() -> p2m_pt_set_entry() ->
>> write_p2m_entry() -> p2m_flush_nestedp2m() call sequence triggers a lock
>> order violation when the PoD lock is held around it. Hence such flushing
>> needs to be deferred. Steal the approach from p2m_change_type_range().
>> (Note that strictly speaking the change at the out_of_memory label is
>> not needed, as the domain gets crashed there anyway. The change is being
>> made nevertheless to avoid setting up a trap from someone meaning to
>> deal with that case better than by domain_crash().)
>>
>> Similarly for EPT I think ept_set_entry() -> ept_sync_domain() ->
>> ept_sync_domain_prepare() -> p2m_flush_nestedp2m() is affected. Make its
>> p2m_flush_nestedp2m() invocation conditional. Note that this then also
>> alters behavior of p2m_change_type_range() on EPT, deferring the nested
>> flushes there as well. I think this should have been that way from the
>> introduction of the flag.
>>
>> Reported-by: Elliott Mitchell <ehem+xen@xxxxxxx>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks.

>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1253,7 +1253,7 @@ static void ept_sync_domain_prepare(stru
>>      {
>>          if ( p2m_is_nestedp2m(p2m) )
>>              ept = &p2m_get_hostp2m(d)->ept;
>> -        else
>> +        else if ( !p2m->defer_nested_flush )
>>              p2m_flush_nestedp2m(d);
> 
> I find this model slightly concerning, as we don't actually notify the
> caller that a nested flush as been deferred, so we must make sure that
> whoever sets defer_nested_flush also performs a flush unconditionally
> when clearing the flag.

Well, this _is_ the model used for now. Until this change there was
just a single party setting the flag. And like here, any new party
setting the flag will also need to invoke a flush upon clearing it.
It's not clear to me what alternative model you may have in mind.

Jan